From mboxrd@z Thu Jan  1 00:00:00 1970
From: jeremie.galarneau@efficios.com (=?UTF-8?Q?J=C3=A9r=C3=A9mie_Galarneau?=)
Date: Wed, 18 May 2016 14:27:00 -0400
Subject: [lttng-dev] [PATCH lttng-tools] Fix: Deference after null check
	in sessiond set_option
In-Reply-To: <1463491966-25175-1-git-send-email-mathieu.desnoyers@efficios.com>
References: <1463491966-25175-1-git-send-email-mathieu.desnoyers@efficios.com>
Message-ID: <CA+jJMxunGErf9nOZPBwDb3STUG-SHWvowa1AJpe_oDzFF_dmYw@mail.gmail.com>

On Tue, May 17, 2016 at 9:32 AM, Mathieu Desnoyers
<mathieu.desnoyers at efficios.com> wrote:
> Found by Coverity:
>
> 2. var_compare_op: Comparing arg to null implies that arg might be null.
>
> CID 1256137 (#1 of 9): Dereference after null check (FORWARD_NULL)14.
> var_deref_model: Passing null pointer arg to strdup, which dereferences
> it.
>
> [... same for #2 through #9 ]
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> ---
>  src/bin/lttng-sessiond/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c
> index 6081fb2..bdf2a74 100644
> --- a/src/bin/lttng-sessiond/main.c
> +++ b/src/bin/lttng-sessiond/main.c
> @@ -4671,7 +4671,7 @@ static int set_option(int opt, const char *arg, const char *optname)
>  {
>         int ret = 0;
>
> -       if (arg && arg[0] == '\0') {
> +       if (!arg || arg[0] == '\0') {

This won't work with options that require an argument (--deamonize,
--verbose, etc.).

I have merged an alternative fix as:

commit 66b2ce8e87e53e5f69446a70b3a4d90399095930
Author: Jérémie Galarneau <jeremie.galarneau at efficios.com>
Date:   Wed May 18 13:20:08 2016 -0400

    Fix: Deference after null check in sessiond set_option

    Found by Coverity:

    2. var_compare_op: Comparing arg to null implies that arg might be null.

    CID 1256137 (#1 of 9): Dereference after null check (FORWARD_NULL)14.
    var_deref_model: Passing null pointer arg to strdup, which dereferences
    it.

    [... same for #2 through #9 ]

    This should not really be an issue since
    1) options that use the "arg" parameter will not be set by popt if one
       is not provided,
    2) the configuration file parser will never invoke set_option with
       a NULL argument; if no "value" is provided in the file, an empty
       string is passed.

    The second point is the reason for the "arg && arg[0] == '\0'" check;
    we already know that the argument is invalid since an empty string
    is never a valid argument for the supported options.

    Nonetheless, it makes sense for Coverity to flag this and moving
    the check to individual cases, although very verbose, is clear.

    Signed-off-by: Jérémie Galarneau <jeremie.galarneau at efficios.com>

Thanks,
Jérémie



>                 /*
>                  * This only happens if the value is read from daemon config
>                  * file. This means the option requires an argument and the
> --
> 2.1.4
>



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