Hi,
On 2017-11-30 10:43 AM, Jérémie Galarneau wrote:
> Hi!
>
> I'm proposing this diff on top of your patch, read on.
>
> diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
> index f921621b..1eaf9bef 100644
> --- a/src/bin/lttng-sessiond/main.c
> +++ b/src/bin/lttng-sessiond/main.c
> @@ -2442,7 +2442,7 @@ static pid_t spawn_consumerd(struct
> consumer_data *consumer_data)
> {
> char *tmpnew = NULL;
>
> - if (config.consumerd64_lib_dir.value &&
> config.consumerd64_lib_dir.value[0] != '\0') {
> + if (config.consumerd64_lib_dir.value) {
> char *tmp;
> size_t tmplen;
>
> @@ -2476,16 +2476,14 @@ static pid_t spawn_consumerd(struct
> consumer_data *consumer_data)
> "--consumerd-err-sock",
> consumer_data->err_unix_sock_path,
> "--group",
> config.tracing_group_name.value,
> NULL);
> - if (config.consumerd64_lib_dir.value[0] != '\0') {
> - free(tmpnew);
> - }
> + free(tmpnew);
> break;
> }
> case LTTNG_CONSUMER32_UST:
> {
> char *tmpnew = NULL;
>
> - if (config.consumerd32_lib_dir.value &&
> config.consumerd32_lib_dir.value[0] != '\0') {
> + if (config.consumerd32_lib_dir.value) {
> char *tmp;
> size_t tmplen;
>
> @@ -2519,9 +2517,7 @@ static pid_t spawn_consumerd(struct
> consumer_data *consumer_data)
> "--consumerd-err-sock",
> consumer_data->err_unix_sock_path,
> "--group",
> config.tracing_group_name.value,
> NULL);
> - if (config.consumerd32_lib_dir.value[0] != '\0') {
> - free(tmpnew);
> - }
> + free(tmpnew);
> break;
> }
> default:
>
> That's a good catch. The same unchecked accesses are performed before
> calling "free(tmpnew)". However, the check is superfluous since tmpnew
> would be NULL anyway.
Weird, why did I did not catch that...
>
> FYI, since e6142f2e, I did away with most of the the val[0] != '\0'
> nonsense that was done on string configuration options. Hence, in this
> instance, only checking for NULL is necessary.
Okai
>
> Looking at the rest of that function, I see that putenv() is used with
> a dynamically allocated string (tmpnew) that is then free'd... That's
> broken. I'll add another fix on top (probably using setenv() to copy
> to the env).
Well good catch! Looks like I was more interested in an immediate fix.
>
> I'll edit and push your patch if that's okay with you?
Don't see any problem with that.
>
> Thanks!
> Jérémie
>
>
> On 29 November 2017 at 22:42, Jonathan Rajotte
> <jonathan.rajotte-jul...@efficios.com> wrote:
>> Reproducer:
>> lttng-sessiond \
>> --consumerd32-path=/usr/local/lib/lttng/libexec/lttng-consumerd \
>> --consumerd64-path=/usr/local/lib/lttng/libexec/lttng-consumerd
>>
>> lttng create
>> lttng enable-event -u -a
>>
>> On a 64bit machine the invocation of the 64bit consumerd will not fail
>> since its libdir is populated by sessiond_config_init but will segfault on
>> spawning of the 32 bit consumerd when performing the check of libdir
>> value.
>>
>> On a 32bit machine the opposite will happen.
>>
>> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-jul...@efficios.com>
>> ---
>>
>> Another possibility would be to always initialize to an empty "" instead of
>> NULL.
>>
>> ---
>> src/bin/lttng-sessiond/main.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
>> index 3d0a65de..f921621b 100644
>> --- a/src/bin/lttng-sessiond/main.c
>> +++ b/src/bin/lttng-sessiond/main.c
>> @@ -2442,7 +2442,7 @@ static pid_t spawn_consumerd(struct consumer_data
>> *consumer_data)
>> {
>> char *tmpnew = NULL;
>>
>> - if (config.consumerd64_lib_dir.value[0] != '\0') {
>> + if (config.consumerd64_lib_dir.value &&
>> config.consumerd64_lib_dir.value[0] != '\0') {
>> char *tmp;
>> size_t tmplen;
>>
>> @@ -2485,7 +2485,7 @@ static pid_t spawn_consumerd(struct consumer_data
>> *consumer_data)
>> {
>> char *tmpnew = NULL;
>>
>> - if (config.consumerd32_lib_dir.value[0] != '\0') {
>> + if (config.consumerd32_lib_dir.value &&
>> config.consumerd32_lib_dir.value[0] != '\0') {
>> char *tmp;
>> size_t tmplen;
>>
>> --
>> 2.11.0
>>
>
>
>
--
Jonathan R. Julien
EfficiOS
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
