[PATCH] libwbclient: wbcSidsToUnixIds() don't fail on errors

[Sumit Bose]

Hi,

this patch fixes an issue in SSSD's implementation of libwbclient.
wbcSidsToUnixIds() translates a list of SID to POSIX IDs and it is
expected that if one SID cannot be mapped the related output entry
should just get type WBC_ID_TYPE_NOT_SPECIFIED. Currently the request
fail completely if one SID cannot be mapped.

To test, use 'wbinfo --sids-to-unix-ids' with an invalid SID.

Without fix:

    $ wbinfo
--sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
    wbcSidsToUnixIds failed: WBC_ERR_UNKNOWN_FAILURE
    wbinfo_sids_to_unix_ids failed

With fix:

    $ wbinfo
--sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
    S-1-5-21-3692237560-1981608775-3610128199-1104 -> uid 700201104
    S-1-5-21-3692237560-1981608775-3610128199-5 -> unmapped

Even with completely random SIDs you should see a proper output:

    $ wbinfo --sids-to-unix-ids=S-2-3-4,S-5-6-7
    S-2-3-4 -> unmapped
    S-5-6-7 -> unmapped

bye,
Sumit

[Alexander Bokovoy]

On Fri, 03 Jun 2016, Sumit Bose wrote:
> Hi,
>
> this patch fixes an issue in SSSD's implementation of libwbclient.
> wbcSidsToUnixIds() translates a list of SID to POSIX IDs and it is
> expected that if one SID cannot be mapped the related output entry
> should just get type WBC_ID_TYPE_NOT_SPECIFIED. Currently the request
> fail completely if one SID cannot be mapped.
>
> To test, use 'wbinfo --sids-to-unix-ids' with an invalid SID.
>
> Without fix:
>
>     $ wbinfo
> --sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
>     wbcSidsToUnixIds failed: WBC_ERR_UNKNOWN_FAILURE
>     wbinfo_sids_to_unix_ids failed
>
> With fix:
>
>     $ wbinfo
> --sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
>     S-1-5-21-3692237560-1981608775-3610128199-1104 -> uid 700201104
>     S-1-5-21-3692237560-1981608775-3610128199-5 -> unmapped
>
> Even with completely random SIDs you should see a proper output:
>
>     $ wbinfo --sids-to-unix-ids=S-2-3-4,S-5-6-7
>     S-2-3-4 -> unmapped
>     S-5-6-7 -> unmapped
>
> bye,
> Sumit 
> From 52de39e45829ffd1bd18b3f83310066f97a38397 Mon Sep 17 00:00:00
> 2001
> From: Sumit Bose <sbose(a)redhat.com&gt;
> Date: Thu, 2 Jun 2016 21:01:11 +0200
> Subject: [PATCH] libwbclient: wbcSidsToUnixIds() don't fail on errors
>
> Resolves: https://fedorahosted.org/sssd/ticket/3028
> ---
>  src/sss_client/libwbclient/wbc_idmap_sssd.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/src/sss_client/libwbclient/wbc_idmap_sssd.c
> b/src/sss_client/libwbclient/wbc_idmap_sssd.c
> index 1b0e2e10a5ce1a0c7577d391b740ff988f920903..b3e292217e056dde323c82f1303b49a058933dab
> 100644
> --- a/src/sss_client/libwbclient/wbc_idmap_sssd.c
> +++ b/src/sss_client/libwbclient/wbc_idmap_sssd.c
> @@ -173,14 +173,12 @@ wbcErr wbcSidsToUnixIds(const struct wbcDomainSid *sids, uint32_t
> num_sids,
>
>      for (c = 0; c < num_sids; c++) {
>          wbc_status = wbcSidToString(&sids[c], &sid_str);
> -        if (!WBC_ERROR_IS_OK(wbc_status)) {
> -            return wbc_status;
> -        }
> -
> -        ret = sss_nss_getidbysid(sid_str, &id, &type);
> -        wbcFreeMemory(sid_str);
> -        if (ret != 0) {
> -            return WBC_ERR_UNKNOWN_FAILURE;
> +        if (WBC_ERROR_IS_OK(wbc_status)) {
> +            ret = sss_nss_getidbysid(sid_str, &id, &type);
> +            wbcFreeMemory(sid_str);
> +            if (ret != 0) {
> +                type = SSS_ID_TYPE_NOT_SPECIFIED;
> +            }
>          }
>
>          switch (type) { 
With this change 'type' variable will become
undefined if wbcSidToString() failed.
Perhaps, it could be set to 'type = SSS_ID_TYPE_NOT_SPECIFIED;' at the
beginning of the for() loop?


-- 
/ Alexander Bokovoy

[Sumit Bose]

On Sun, Jun 05, 2016 at 10:36:51PM +0300, Alexander Bokovoy wrote:
>  On Fri, 03 Jun 2016, Sumit Bose wrote:
>  > Hi,
>  > 
>  > this patch fixes an issue in SSSD's implementation of libwbclient.
>  > wbcSidsToUnixIds() translates a list of SID to POSIX IDs and it is
>  > expected that if one SID cannot be mapped the related output entry
>  > should just get type WBC_ID_TYPE_NOT_SPECIFIED. Currently the request
>  > fail completely if one SID cannot be mapped.
>  > 
>  > To test, use 'wbinfo --sids-to-unix-ids' with an invalid SID.
>  > 
>  > Without fix:
>  > 
>  >    $ wbinfo
> --sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
>  >    wbcSidsToUnixIds failed: WBC_ERR_UNKNOWN_FAILURE
>  >    wbinfo_sids_to_unix_ids failed
>  > 
>  > With fix:
>  > 
>  >    $ wbinfo
> --sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
>  >    S-1-5-21-3692237560-1981608775-3610128199-1104 -> uid 700201104
>  >    S-1-5-21-3692237560-1981608775-3610128199-5 -> unmapped
>  > 
>  > Even with completely random SIDs you should see a proper output:
>  > 
>  >    $ wbinfo --sids-to-unix-ids=S-2-3-4,S-5-6-7
>  >    S-2-3-4 -> unmapped
>  >    S-5-6-7 -> unmapped
>  > 
>  > bye,
>  > Sumit
>  
>  > From 52de39e45829ffd1bd18b3f83310066f97a38397 Mon Sep 17 00:00:00 2001
>  > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > Date: Thu, 2 Jun 2016 21:01:11 +0200
>  > Subject: [PATCH] libwbclient: wbcSidsToUnixIds() don't fail on errors
>  > 
>  > Resolves: https://fedorahosted.org/sssd/ticket/3028
>  > ---
>  > src/sss_client/libwbclient/wbc_idmap_sssd.c | 14 ++++++--------
>  > 1 file changed, 6 insertions(+), 8 deletions(-)
>  > 
>  > diff --git a/src/sss_client/libwbclient/wbc_idmap_sssd.c
> b/src/sss_client/libwbclient/wbc_idmap_sssd.c
>  > index
> 1b0e2e10a5ce1a0c7577d391b740ff988f920903..b3e292217e056dde323c82f1303b49a058933dab 100644
>  > --- a/src/sss_client/libwbclient/wbc_idmap_sssd.c
>  > +++ b/src/sss_client/libwbclient/wbc_idmap_sssd.c
>  > @@ -173,14 +173,12 @@ wbcErr wbcSidsToUnixIds(const struct wbcDomainSid *sids,
> uint32_t num_sids,
>  > 
>  >     for (c = 0; c < num_sids; c++) {
>  >         wbc_status = wbcSidToString(&sids[c], &sid_str);
>  > -        if (!WBC_ERROR_IS_OK(wbc_status)) {
>  > -            return wbc_status;
>  > -        }
>  > -
>  > -        ret = sss_nss_getidbysid(sid_str, &id, &type);
>  > -        wbcFreeMemory(sid_str);
>  > -        if (ret != 0) {
>  > -            return WBC_ERR_UNKNOWN_FAILURE;
>  > +        if (WBC_ERROR_IS_OK(wbc_status)) {
>  > +            ret = sss_nss_getidbysid(sid_str, &id, &type);
>  > +            wbcFreeMemory(sid_str);
>  > +            if (ret != 0) {
>  > +                type = SSS_ID_TYPE_NOT_SPECIFIED;
>  > +            }
>  >         }
>  > 
>  >         switch (type) {
>  With this change 'type' variable will become undefined if wbcSidToString()
> failed.
>  Perhaps, it could be set to 'type = SSS_ID_TYPE_NOT_SPECIFIED;' at the
>  beginning of the for() loop? 
Hi Alexander,

thank you for catching this. I have added your suggestion to the patch.
Please note that although not strictly needed I kept the 'if (ret != 0)'
block to avoid compiler or Coverity warnings about unchecked return
values.

bye,
Sumit

>  
>  
>  -- 
>  / Alexander Bokovoy
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/admin/lists/[e-mail]

[Alexander Bokovoy]

On Mon, 06 Jun 2016, Sumit Bose wrote:
> On Sun, Jun 05, 2016 at 10:36:51PM +0300, Alexander Bokovoy wrote:
> > On Fri, 03 Jun 2016, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch fixes an issue in SSSD's implementation of libwbclient.
> > > wbcSidsToUnixIds() translates a list of SID to POSIX IDs and it is
> > > expected that if one SID cannot be mapped the related output entry
> > > should just get type WBC_ID_TYPE_NOT_SPECIFIED. Currently the request
> > > fail completely if one SID cannot be mapped.
> > >
> > > To test, use 'wbinfo --sids-to-unix-ids' with an invalid SID.
> > >
> > > Without fix:
> > >
> > >    $ wbinfo
> --sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
> > >    wbcSidsToUnixIds failed: WBC_ERR_UNKNOWN_FAILURE
> > >    wbinfo_sids_to_unix_ids failed
> > >
> > > With fix:
> > >
> > >    $ wbinfo
> --sids-to-unix-ids=S-1-5-21-3692237560-1981608775-3610128199-1104,S-1-5-21-3692237560-1981608775-3610128199-5
> > >    S-1-5-21-3692237560-1981608775-3610128199-1104 -> uid 700201104
> > >    S-1-5-21-3692237560-1981608775-3610128199-5 -> unmapped
> > >
> > > Even with completely random SIDs you should see a proper output:
> > >
> > >    $ wbinfo --sids-to-unix-ids=S-2-3-4,S-5-6-7
> > >    S-2-3-4 -> unmapped
> > >    S-5-6-7 -> unmapped
> > >
> > > bye,
> > > Sumit
> >
> > > From 52de39e45829ffd1bd18b3f83310066f97a38397 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com&gt;
> > > Date: Thu, 2 Jun 2016 21:01:11 +0200
> > > Subject: [PATCH] libwbclient: wbcSidsToUnixIds() don't fail on errors
> > >
> > > Resolves: https://fedorahosted.org/sssd/ticket/3028
> > > ---
> > > src/sss_client/libwbclient/wbc_idmap_sssd.c | 14 ++++++--------
> > > 1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/sss_client/libwbclient/wbc_idmap_sssd.c
> b/src/sss_client/libwbclient/wbc_idmap_sssd.c
> > > index
> 1b0e2e10a5ce1a0c7577d391b740ff988f920903..b3e292217e056dde323c82f1303b49a058933dab 100644
> > > --- a/src/sss_client/libwbclient/wbc_idmap_sssd.c
> > > +++ b/src/sss_client/libwbclient/wbc_idmap_sssd.c
> > > @@ -173,14 +173,12 @@ wbcErr wbcSidsToUnixIds(const struct wbcDomainSid *sids,
> uint32_t num_sids,
> > >
> > >     for (c = 0; c < num_sids; c++) {
> > >         wbc_status = wbcSidToString(&sids[c], &sid_str);
> > > -        if (!WBC_ERROR_IS_OK(wbc_status)) {
> > > -            return wbc_status;
> > > -        }
> > > -
> > > -        ret = sss_nss_getidbysid(sid_str, &id, &type);
> > > -        wbcFreeMemory(sid_str);
> > > -        if (ret != 0) {
> > > -            return WBC_ERR_UNKNOWN_FAILURE;
> > > +        if (WBC_ERROR_IS_OK(wbc_status)) {
> > > +            ret = sss_nss_getidbysid(sid_str, &id, &type);
> > > +            wbcFreeMemory(sid_str);
> > > +            if (ret != 0) {
> > > +                type = SSS_ID_TYPE_NOT_SPECIFIED;
> > > +            }
> > >         }
> > >
> > >         switch (type) {
> > With this change 'type' variable will become undefined if wbcSidToString()
> failed.
> > Perhaps, it could be set to 'type = SSS_ID_TYPE_NOT_SPECIFIED;' at the
> > beginning of the for() loop?
>
> Hi Alexander,
>
> thank you for catching this. I have added your suggestion to the patch.
> Please note that although not strictly needed I kept the 'if (ret != 0)'
> block to avoid compiler or Coverity warnings about unchecked return
> values. 
Yes, I was thinking how to merge those two yesterday as well and also
decided that the 'if (ret != 0)' should stay.

ACK.

-- 
/ Alexander Bokovoy

[Jakub Hrozek]

On Mon, Jun 06, 2016 at 03:22:22PM +0300, Alexander Bokovoy wrote:
>  ACK. 
master: 52f1093ef3d7c44132ec10c57436865b2cbb19d7
sssd-1-13: 15ad5f603a5797c61a01f67365c2581c7bddcdfa