[PATCH] s3/winbindd: use == -1 instead of < 0 for error checking uid_t
Michael Adam
obnox at samba.org
Thu Jun 30 23:02:57 UTC 2016
On 2016-06-30 at 15:52 -0700, Jeremy Allison wrote:
> On Wed, Mar 09, 2016 at 04:39:05PM +0100, Aurélien Aptel wrote:
> > Hi,
> >
> > Another PVS fix.
> >
> > The sign of the uid_t type is left unspecified by POSIX. It's defined as
> > an unsigned 32b int on Linux, therefore the < 0 check is always
> > false.
> >
> > For unsigned version of uid_t, "uid == -1" will implicitely cast -1 to
> > unsigned making it a valid test for both signed and unsigned version of
> > uid_t.
> >
> > This commit makes the cast to uid_t explicit anyway. I think less
> > magic is better in this case.
> >
> > Please review&push.
>
> Sorry for the delay. This LGTM.
>
> Reviewed-by: Jeremy Allison <jra at samba.org>
>
> Can I get a second Team reviewer ?
Generally, reviewed-by me.
Two minor comments, which you might or might not
want to address:
- I would support having two patches: one for each file.
- In the first hunk of the winbindd_pam patch, one
could even spare the "return -1;" with this patch :-)
Cheers - Michael
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
> > SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> > GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
> > Nürnberg)
>
> > From 98396235dfc3da614d86ccab72c8df32891479f1 Mon Sep 17 00:00:00 2001
> > From: Aurelien Aptel <aaptel at suse.com>
> > Date: Wed, 9 Mar 2016 13:43:09 +0100
> > Subject: [PATCH] s3/winbindd: use == -1 instead of < 0 for error checking
> > uid_t
> >
> > The sign of the uid_t type is left unspecified by POSIX. It's defined as
> > an unsigned 32b int on Linux, therefore the < 0 check is always
> > false.
> >
> > For unsigned version of uid_t, "uid == -1" will implicitely cast -1 to
> > unsigned making it a valid test for both signed and unsigned version of
> > uid_t.
> >
> > This commit makes the cast to (uid_t) explicit anyway.
> >
> > Signed-off-by: Aurelien Aptel <aaptel at suse.com>
> > ---
> > source3/winbindd/winbindd_cred_cache.c | 2 +-
> > source3/winbindd/winbindd_pam.c | 4 ++--
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/source3/winbindd/winbindd_cred_cache.c b/source3/winbindd/winbindd_cred_cache.c
> > index e113f99..20b4d55 100644
> > --- a/source3/winbindd/winbindd_cred_cache.c
> > +++ b/source3/winbindd/winbindd_cred_cache.c
> > @@ -503,7 +503,7 @@ NTSTATUS add_ccache_to_list(const char *princ_name,
> > NTSTATUS ntret;
> >
> > if ((username == NULL && princ_name == NULL) ||
> > - ccname == NULL || uid < 0) {
> > + ccname == NULL || uid == (uid_t)-1) {
> > return NT_STATUS_INVALID_PARAMETER;
> > }
> >
> > diff --git a/source3/winbindd/winbindd_pam.c b/source3/winbindd/winbindd_pam.c
> > index 8910423..67a00e4 100644
> > --- a/source3/winbindd/winbindd_pam.c
> > +++ b/source3/winbindd/winbindd_pam.c
> > @@ -555,7 +555,7 @@ uid_t get_uid_from_request(struct winbindd_request *request)
> >
> > uid = request->data.auth.uid;
> >
> > - if (uid < 0) {
> > + if (uid == (uid_t)-1) {
> > DEBUG(1,("invalid uid: '%u'\n", (unsigned int)uid));
> > return -1;
> > }
> > @@ -2302,7 +2302,7 @@ enum winbindd_result winbindd_dual_pam_logoff(struct winbindd_domain *domain,
> >
> > #ifdef HAVE_KRB5
> >
> > - if (state->request->data.logoff.uid < 0) {
> > + if (state->request->data.logoff.uid == (uid_t)-1) {
> > DEBUG(0,("winbindd_pam_logoff: invalid uid\n"));
> > goto process_result;
> > }
> > --
> > 2.1.4
> >
>
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160701/7ec0af3c/signature.sig>
More information about the samba-technical
mailing list