[PATCH] proxy: Do not try to store same alias twice

[Michal Židek]

I found this bug while working on
https://fedorahosted.org/sssd/ticket/2461

It turned out that not only case_sensitive = preserving,
but also case_sensitive = false did not work
with proxy provider.

But this patch does not solve the issue in the ticket,
so I did not link them.

Michal

[Michal Židek]

On 11/05/2014 04:53 PM, Michal Židek wrote:
>  I found this bug while working on
>  https://fedorahosted.org/sssd/ticket/2461
>
>  It turned out that not only case_sensitive = preserving,
>  but also case_sensitive = false did not work
>  with proxy provider.
>
>  But this patch does not solve the issue in the ticket,
>  so I did not link them. 
After  some investigation I actually think this was
the main issue that caused the bug mentioned in
the ticket so I added the "Fixes" label to it.

I also added second patch that preservers the
service name with proxy provider.

Michal

[Michal Židek]

On 11/06/2014 07:43 PM, Michal Židek wrote:
>  On 11/05/2014 04:53 PM, Michal Židek wrote:
> > I found this bug while working on
> > https://fedorahosted.org/sssd/ticket/2461
> >
> > It turned out that not only case_sensitive = preserving,
> > but also case_sensitive = false did not work
> > with proxy provider.
> >
> > But this patch does not solve the issue in the ticket,
> > so I did not link them.
>
>  After  some investigation I actually think this was
>  the main issue that caused the bug mentioned in
>  the ticket so I added the "Fixes" label to it.
>
>  I also added second patch that preservers the
>  service name with proxy provider.
>
>  Michal
>  
And now with patches :)

[Jakub Hrozek]

On Thu, Nov 06, 2014 at 07:48:20PM +0100, Michal Židek wrote:
>  On 11/06/2014 07:43 PM, Michal Židek wrote:
>  >On 11/05/2014 04:53 PM, Michal Židek wrote:
>  >>I found this bug while working on
>  >>https://fedorahosted.org/sssd/ticket/2461
>  >>
>  >>It turned out that not only case_sensitive = preserving,
>  >>but also case_sensitive = false did not work
>  >>with proxy provider.
>  >>
>  >>But this patch does not solve the issue in the ticket,
>  >>so I did not link them.
>  >
>  >After  some investigation I actually think this was
>  >the main issue that caused the bug mentioned in
>  >the ticket so I added the "Fixes" label to it.
>  >
>  >I also added second patch that preservers the
>  >service name with proxy provider.
>  >
>  >Michal
>  >
>  
>  And now with patches :)
>   
Coverity found a warning in the patches:
Error: UNUSED_VALUE (CWE-563):
sssd-1.12.3/src/providers/proxy/proxy_id.c:609: value_overwrite: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
overwritten with value "12".
sssd-1.12.3/src/providers/proxy/proxy_id.c:614: value_overwrite: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
overwritten with value from "sysdb_attrs_add_string(attrs, "nameAlias",
cased_alias)".
sssd-1.12.3/src/providers/proxy/proxy_id.c:623: value_overwrite: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
overwritten with value from "sysdb_store_group(dom, real_name, grp->gr_gid, attrs,
cache_timeout, now)".
sssd-1.12.3/src/providers/proxy/proxy_id.c:603: returned_value: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is assigned
to "ret" here, but that stored value is not used before it is overwritten.

Looks like an unchecked call. Apart from that, the code looks good.

[Pavel Reichl]

On 11/06/2014 07:48 PM, Michal Židek wrote:
>  0001-proxy-Do-not-try-to-store-same-alias-twice.patch
>
>
>   From f6c84c306cf921344ad2b23040588040ac407186 Mon Sep 17 00:00:00 2001
>  From: Michal Zidek<mzidek(a)redhat.com>
>  Date: Fri, 31 Oct 2014 16:39:25 +0100
>  Subject: [PATCH 1/2] proxy: Do not try to store same alias twice
>
>  LDB does not store attributes if they have the
>  same name and value and errors out instead.
>
>  Fixes:
>  https://fedorahosted.org/sssd/ticket/2461
>  ---
>    src/providers/proxy/proxy_id.c | 68 ++++++++++++++++++++++++++----------------
>    1 file changed, 42 insertions(+), 26 deletions(-)
>
>  diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
>  index d867ec4..bb79202 100644
>  --- a/src/providers/proxy/proxy_id.c
>  +++ b/src/providers/proxy/proxy_id.c
>  @@ -222,6 +222,7 @@ static int save_user(struct sss_domain_info *domain,
>        struct sysdb_attrs *attrs = NULL;
>        errno_t ret;
>        const char *cased_alias;
>  +    const char *lc_pw_name = NULL;
>    
>        if (pwd->pw_shell && pwd->pw_shell[0] != '\0') {
>            shell = pwd->pw_shell;
>  @@ -239,31 +240,40 @@ static int save_user(struct sss_domain_info *domain,
>            attrs = sysdb_new_attrs(NULL);
>            if (!attrs) {
>                DEBUG(SSSDBG_CRIT_FAILURE, "Allocation error ?!\n");
>  -            return ENOMEM;
>  +            ret = ENOMEM;
>  +            goto done;
>            }
>        }
>    
>        if (lowercase) {
>  -        ret = sysdb_attrs_add_lc_name_alias(attrs, pwd->pw_name);
>  +        lc_pw_name = sss_tc_utf8_str_tolower(attrs, pwd->pw_name);
>  +        if (lc_pw_name == NULL) {
>  +            DEBUG(SSSDBG_OP_FAILURE, "Cannot convert name to lowercase.\n");
>  +            ret = ENOMEM;
>  +            goto done;
>  +        }
>  +
>  +        ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, lc_pw_name);
>            if (ret) {
>                DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n");
>  -            talloc_zfree(attrs);
>  -            return ret;
>  +            goto done;
>            }
>  +
>        }
>    
>        if (alias) {
>            cased_alias = sss_get_cased_name(attrs, alias, !lowercase);
>            if (!cased_alias) {
>  -            talloc_zfree(attrs);
>  -            return ENOMEM; 
ret = ENOMEM;
>  +            goto done;
>            }
>    
>  -        ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, cased_alias);
>  -        if (ret) {
>  -            DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n");
>  -            talloc_zfree(attrs);
>  -            return ret;
>  +        /* Add the alias only if it differs from lowercased pw_name */
>  +        if (lc_pw_name == NULL || strcmp(cased_alias, lc_pw_name) != 0) {
>  +            ret = sysdb_attrs_add_string(attrs, SYSDB_NAME_ALIAS, cased_alias);
>  +            if (ret) {
>  +                DEBUG(SSSDBG_OP_FAILURE, "Could not add name alias\n");
>  +                goto done;
>  +            }
>            }
>        }
>    
>  @@ -280,12 +290,13 @@ static int save_user(struct sss_domain_info *domain,
>                               NULL,
>                               cache_timeout,
>                               0);
>  -    talloc_zfree(attrs);
>        if (ret) {
>            DEBUG(SSSDBG_OP_FAILURE, "Could not add user to cache\n");
>  -        return ret;
>  +        goto done;
>        }
>    
>  +done:
>  +    talloc_zfree(attrs);
>        return EOK; 
Did you mean "return ret"?
>    }

[Michal Židek]

On 11/11/2014 01:37 PM, Jakub Hrozek wrote:
>  On Thu, Nov 06, 2014 at 07:48:20PM +0100, Michal Židek wrote:
> > On 11/06/2014 07:43 PM, Michal Židek wrote:
> >> On 11/05/2014 04:53 PM, Michal Židek wrote:
> >>> I found this bug while working on
> >>> https://fedorahosted.org/sssd/ticket/2461
> >>>
> >>> It turned out that not only case_sensitive = preserving,
> >>> but also case_sensitive = false did not work
> >>> with proxy provider.
> >>>
> >>> But this patch does not solve the issue in the ticket,
> >>> so I did not link them.
> >>
> >> After  some investigation I actually think this was
> >> the main issue that caused the bug mentioned in
> >> the ticket so I added the "Fixes" label to it.
> >>
> >> I also added second patch that preservers the
> >> service name with proxy provider.
> >>
> >> Michal
> >>
> >
> > And now with patches :)
> >
>
>  Coverity found a warning in the patches:
>  Error: UNUSED_VALUE (CWE-563):
>  sssd-1.12.3/src/providers/proxy/proxy_id.c:609: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value "12".
>  sssd-1.12.3/src/providers/proxy/proxy_id.c:614: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_attrs_add_string(attrs, "nameAlias",
> cased_alias)".
>  sssd-1.12.3/src/providers/proxy/proxy_id.c:623: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_store_group(dom, real_name, grp->gr_gid, attrs,
> cache_timeout, now)".
>  sssd-1.12.3/src/providers/proxy/proxy_id.c:603: returned_value: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is assigned
> to "ret" here, but that stored value is not used before it is overwritten.
>
>  Looks like an unchecked call. Apart from that, the code looks good. 
Thank you Jakub and Pavel for the review. New patches are attached.

Michal

[Jakub Hrozek]

On Wed, Nov 12, 2014 at 02:53:00PM +0100, Michal Židek wrote:
>  On 11/11/2014 01:37 PM, Jakub Hrozek wrote:
>  >On Thu, Nov 06, 2014 at 07:48:20PM +0100, Michal Židek wrote:
>  >>On 11/06/2014 07:43 PM, Michal Židek wrote:
>  >>>On 11/05/2014 04:53 PM, Michal Židek wrote:
>  >>>>I found this bug while working on
>  >>>>https://fedorahosted.org/sssd/ticket/2461
>  >>>>
>  >>>>It turned out that not only case_sensitive = preserving,
>  >>>>but also case_sensitive = false did not work
>  >>>>with proxy provider.
>  >>>>
>  >>>>But this patch does not solve the issue in the ticket,
>  >>>>so I did not link them.
>  >>>
>  >>>After  some investigation I actually think this was
>  >>>the main issue that caused the bug mentioned in
>  >>>the ticket so I added the "Fixes" label to it.
>  >>>
>  >>>I also added second patch that preservers the
>  >>>service name with proxy provider.
>  >>>
>  >>>Michal
>  >>>
>  >>
>  >>And now with patches :)
>  >>
>  >
>  >Coverity found a warning in the patches:
>  >Error: UNUSED_VALUE (CWE-563):
>  >sssd-1.12.3/src/providers/proxy/proxy_id.c:609: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value "12".
>  >sssd-1.12.3/src/providers/proxy/proxy_id.c:614: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_attrs_add_string(attrs, "nameAlias",
> cased_alias)".
>  >sssd-1.12.3/src/providers/proxy/proxy_id.c:623: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_store_group(dom, real_name, grp->gr_gid, attrs,
> cache_timeout, now)".
>  >sssd-1.12.3/src/providers/proxy/proxy_id.c:603: returned_value: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is assigned
> to "ret" here, but that stored value is not used before it is overwritten.
>  >
>  >Looks like an unchecked call. Apart from that, the code looks good.
>  
>  Thank you Jakub and Pavel for the review. New patches are attached.
>  
>  Michal
>   
>  From fbd49da512a60d18dad11a5f1751b45db7ded168 Mon Sep 17 00:00:00
> 2001
>  From: Michal Zidek <mzidek(a)redhat.com&gt;
>  Date: Fri, 31 Oct 2014 16:39:25 +0100
>  Subject: [PATCH 1/2] proxy: Do not try to store same alias twice
>  
>  LDB does not store attributes if they have the
>  same name and value and errors out instead.
>  
>  Fixes:
>  https://fedorahosted.org/sssd/ticket/2461
>  ---
>   src/providers/proxy/proxy_id.c | 76 ++++++++++++++++++++++++++----------------
>   1 file changed, 48 insertions(+), 28 deletions(-)
>   
[...]

>       if (alias) {
>           cased_alias = sss_get_cased_name(attrs, alias, !lowercase);
>           if (!cased_alias) {
>  -            talloc_zfree(attrs);
>  -            return ENOMEM;
>  +            goto done; 
ret is undefined on this jump (probably would be EOK).

The rest of the code looks good to me.

[Michal Židek]

On 11/13/2014 06:45 PM, Jakub Hrozek wrote:
>  On Wed, Nov 12, 2014 at 02:53:00PM +0100, Michal Židek wrote:
> > On 11/11/2014 01:37 PM, Jakub Hrozek wrote:
> >> On Thu, Nov 06, 2014 at 07:48:20PM +0100, Michal Židek wrote:
> >>> On 11/06/2014 07:43 PM, Michal Židek wrote:
> >>>> On 11/05/2014 04:53 PM, Michal Židek wrote:
> >>>>> I found this bug while working on
> >>>>> https://fedorahosted.org/sssd/ticket/2461
> >>>>>
> >>>>> It turned out that not only case_sensitive = preserving,
> >>>>> but also case_sensitive = false did not work
> >>>>> with proxy provider.
> >>>>>
> >>>>> But this patch does not solve the issue in the ticket,
> >>>>> so I did not link them.
> >>>>
> >>>> After  some investigation I actually think this was
> >>>> the main issue that caused the bug mentioned in
> >>>> the ticket so I added the "Fixes" label to it.
> >>>>
> >>>> I also added second patch that preservers the
> >>>> service name with proxy provider.
> >>>>
> >>>> Michal
> >>>>
> >>>
> >>> And now with patches :)
> >>>
> >>
> >> Coverity found a warning in the patches:
> >> Error: UNUSED_VALUE (CWE-563):
> >> sssd-1.12.3/src/providers/proxy/proxy_id.c:609: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value "12".
> >> sssd-1.12.3/src/providers/proxy/proxy_id.c:614: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_attrs_add_string(attrs, "nameAlias",
> cased_alias)".
> >> sssd-1.12.3/src/providers/proxy/proxy_id.c:623: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_store_group(dom, real_name, grp->gr_gid, attrs,
> cache_timeout, now)".
> >> sssd-1.12.3/src/providers/proxy/proxy_id.c:603: returned_value: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is assigned
> to "ret" here, but that stored value is not used before it is overwritten.
> >>
> >> Looks like an unchecked call. Apart from that, the code looks good.
> >
> > Thank you Jakub and Pavel for the review. New patches are attached.
> >
> > Michal
> >
>
> >  From fbd49da512a60d18dad11a5f1751b45db7ded168 Mon Sep 17 00:00:00 2001
> > From: Michal Zidek <mzidek(a)redhat.com&gt;
> > Date: Fri, 31 Oct 2014 16:39:25 +0100
> > Subject: [PATCH 1/2] proxy: Do not try to store same alias twice
> >
> > LDB does not store attributes if they have the
> > same name and value and errors out instead.
> >
> > Fixes:
> > https://fedorahosted.org/sssd/ticket/2461
> > ---
> >   src/providers/proxy/proxy_id.c | 76 ++++++++++++++++++++++++++----------------
> >   1 file changed, 48 insertions(+), 28 deletions(-)
> >
>
>  [...]
>
> >       if (alias) {
> >           cased_alias = sss_get_cased_name(attrs, alias, !lowercase);
> >           if (!cased_alias) {
> > -            talloc_zfree(attrs);
> > -            return ENOMEM;
> > +            goto done;
>
>  ret is undefined on this jump (probably would be EOK). 
Sorry for that. Fixed now.

>  The rest of the code looks good to me. 
New version is attached. I only changed that one line.

Michal

[Jakub Hrozek]

On Thu, Nov 13, 2014 at 07:00:02PM +0100, Michal Židek wrote:
>  On 11/13/2014 06:45 PM, Jakub Hrozek wrote:
>  >On Wed, Nov 12, 2014 at 02:53:00PM +0100, Michal Židek wrote:
>  >>On 11/11/2014 01:37 PM, Jakub Hrozek wrote:
>  >>>On Thu, Nov 06, 2014 at 07:48:20PM +0100, Michal Židek wrote:
>  >>>>On 11/06/2014 07:43 PM, Michal Židek wrote:
>  >>>>>On 11/05/2014 04:53 PM, Michal Židek wrote:
>  >>>>>>I found this bug while working on
>  >>>>>>https://fedorahosted.org/sssd/ticket/2461
>  >>>>>>
>  >>>>>>It turned out that not only case_sensitive = preserving,
>  >>>>>>but also case_sensitive = false did not work
>  >>>>>>with proxy provider.
>  >>>>>>
>  >>>>>>But this patch does not solve the issue in the ticket,
>  >>>>>>so I did not link them.
>  >>>>>
>  >>>>>After  some investigation I actually think this was
>  >>>>>the main issue that caused the bug mentioned in
>  >>>>>the ticket so I added the "Fixes" label to it.
>  >>>>>
>  >>>>>I also added second patch that preservers the
>  >>>>>service name with proxy provider.
>  >>>>>
>  >>>>>Michal
>  >>>>>
>  >>>>
>  >>>>And now with patches :)
>  >>>>
>  >>>
>  >>>Coverity found a warning in the patches:
>  >>>Error: UNUSED_VALUE (CWE-563):
>  >>>sssd-1.12.3/src/providers/proxy/proxy_id.c:609: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value "12".
>  >>>sssd-1.12.3/src/providers/proxy/proxy_id.c:614: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_attrs_add_string(attrs, "nameAlias",
> cased_alias)".
>  >>>sssd-1.12.3/src/providers/proxy/proxy_id.c:623: value_overwrite: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
> overwritten with value from "sysdb_store_group(dom, real_name, grp->gr_gid, attrs,
> cache_timeout, now)".
>  >>>sssd-1.12.3/src/providers/proxy/proxy_id.c:603: returned_value: Value from
> "sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is assigned
> to "ret" here, but that stored value is not used before it is overwritten.
>  >>>
>  >>>Looks like an unchecked call. Apart from that, the code looks good.
>  >>
>  >>Thank you Jakub and Pavel for the review. New patches are attached.
>  >>
>  >>Michal
>  >>
>  >
>  >> From fbd49da512a60d18dad11a5f1751b45db7ded168 Mon Sep 17 00:00:00 2001
>  >>From: Michal Zidek <mzidek(a)redhat.com&gt;
>  >>Date: Fri, 31 Oct 2014 16:39:25 +0100
>  >>Subject: [PATCH 1/2] proxy: Do not try to store same alias twice
>  >>
>  >>LDB does not store attributes if they have the
>  >>same name and value and errors out instead.
>  >>
>  >>Fixes:
>  >>https://fedorahosted.org/sssd/ticket/2461
>  >>---
>  >>  src/providers/proxy/proxy_id.c | 76 ++++++++++++++++++++++++++----------------
>  >>  1 file changed, 48 insertions(+), 28 deletions(-)
>  >>
>  >
>  >[...]
>  >
>  >>      if (alias) {
>  >>          cased_alias = sss_get_cased_name(attrs, alias, !lowercase);
>  >>          if (!cased_alias) {
>  >>-            talloc_zfree(attrs);
>  >>-            return ENOMEM;
>  >>+            goto done;
>  >
>  >ret is undefined on this jump (probably would be EOK).
>  
>  Sorry for that. Fixed now.
>  
>  >
>  >The rest of the code looks good to me.
>  
>  New version is attached. I only changed that one line.
>  
>  Michal 
ACK

[Jakub Hrozek]

On Thu, Nov 13, 2014 at 07:27:04PM +0100, Jakub Hrozek wrote:
>  > New version is attached. I only changed that one line.
>  > 
>  > Michal
>  
>  ACK 
* master:
    22e074249928605a1d5b926274ae2efb1596bc73
    38429c99bf5af14c2d6bae6ddcf70974fdd103cc