[SSSD] Re: [PATCH] TOOLS: Prevent dereference of null pointer

[Sumit Bose]

On Mon, Mar 07, 2016 at 01:33:38PM +0100, Lukas Slebodnik wrote:
> On (07/03/16 12:12), Pavel Březina wrote:
> >On 03/07/2016 10:14 AM, Lukas Slebodnik wrote:
> >>ehlo,
> >>
> >>simple aptch is attached.
> >
> >When there, can you also talloc_free(attrs) on error? Thanks.
> See updated patch
> 
> LS

> From 7e70c88277d7ae3f7b953af51da4e97917923fda Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lsleb...@redhat.com>
> Date: Sat, 5 Mar 2016 18:57:22 +0100
> Subject: [PATCH] TOOLS: Prevent dereference of null pointer
> 
> VAR_CHECK is called with (var, EOK, ...)
> EOK would be returned in case of "var != EOK"
> and output argument _attrs would not be initialized.
> Therefore there could be dereference of null pointer
> after calling function usermod_build_attrs.

This is just a suggestion to avoid a macro which change the control
flow, i.e. call return or goto. If the code of usermod_build_attrs() is
modified like:

    if (shell) {
        attr_name = SYSDB_SHELL;
        ret = sysdb_attrs_add_string(attrs,
                                     attr_name,
                                     shell);
    }

    if (ret == EOK && home) {
        attr_name = SYSDB_HOMEDIR;
        ret = sysdb_attrs_add_string(attrs,
                                     attr_name,
                                     home);
    }

    .....

    if (ret != EOK) {
        DEBUG(SSSDBG_CRIT_FAILURE, "Could not add attribute [%s] to 
changeset.\n", attr_name);
        goto done;
    }
       

the macro can be avoided completely without adding the debug message for
every attribute.

bye,
Sumit


> ---
>  src/tools/sss_sync_ops.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c
> index 
> 5468929b691c6539cdf55f59be3560412e398f21..b2bbe4c74f5969b097af0f82bb00ca7ec35a596c
>  100644
> --- a/src/tools/sss_sync_ops.c
> +++ b/src/tools/sss_sync_ops.c
> @@ -40,7 +40,7 @@
>  #define VAR_CHECK(var, val, attr, msg) do { \
>          if (var != (val)) { \
>              DEBUG(SSSDBG_CRIT_FAILURE, msg" attribute: %s\n", attr); \
> -            return val; \
> +            goto done; \
>          } \
>  } while(0)
>  
> @@ -266,7 +266,12 @@ static int usermod_build_attrs(TALLOC_CTX *mem_ctx,
>      }
>  
>      *_attrs = attrs;
> -    return EOK;
> +    ret = EOK;
> +done:
> +    if (ret != EOK) {
> +        talloc_free(attrs);
> +    }
> +    return ret;
>  }
>  
>  /*
> -- 
> 2.7.2
> 

> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org