[PATCH] s3/winbindd: use == -1 instead of < 0 for error checking uid_t

[Jeremy Allison]

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 ?

> 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
>