[lttng-dev] [RFC PATCH] Fix: consumerd(64/32)_lib_dir can be NULL

[Jérémie Galarneau]

On 30 November 2017 at 17:10, Jonathan Rajotte Julien
<Jonathan.rajotte-julien at efficios.com> wrote:
> 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.

Alright. Thanks again for the fix!
Merged in master and stable-2.10.

Jérémie

>
>>
>> Thanks!
>> Jérémie
>>
>>
>> On 29 November 2017 at 22:42, Jonathan Rajotte
>> <jonathan.rajotte-julien at 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-julien at 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



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com