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

[Jérémie Galarneau]

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.

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.

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).

I'll edit and push your patch if that's okay with you?

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
>



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