[Pam-developers] Re: Optimize the checkgrouplist function

[Dmitry V. Levin]

On Fri, Oct 18, 2019 at 04:20:55PM +0200, Tomas Mraz wrote:
>  There is no point in doing the resize buffer by doubling for
>  checkgrouplist when ngroups argument should contain the real number of
>  groups.
>  
>  The attached patch implements this optimization. Please review. 
[...]
>  diff --git a/libpam/pam_modutil_ingroup.c
> b/libpam/pam_modutil_ingroup.c
>  index 875cf3e..bc7243a 100644
>  --- a/libpam/pam_modutil_ingroup.c
>  +++ b/libpam/pam_modutil_ingroup.c
>  @@ -12,31 +12,32 @@
>   #include <grp.h>
>   
>   #ifdef HAVE_GETGROUPLIST
>  +
>  +#define NGROUPS_MIN 100
>  +#define NGROUPS_MAX 65536
>  +
>   static int checkgrouplist(const char *user, gid_t primary, gid_t target)
>   {
>   	gid_t *grouplist = NULL; 
This initialization of grouplist was (and remains) misleading:
grouplist is always initialized inside the loop, it is never reset to NULL
after free(grouplist), and it is not used outside the loop.

I'd just moved this pointer inside the loop unless you want
to rewrite this function to use realloc.

>  -	int agroups, ngroups, i;
>  -	ngroups = agroups = 3;
>  +	int ngroups, pgroups, i;
>  +	ngroups = NGROUPS_MIN;
>   	do {
>  -		grouplist = malloc(sizeof(gid_t) * agroups);
>  +		pgroups = ngroups;
>  +		grouplist = malloc(sizeof(gid_t) * ngroups);
>   		if (grouplist == NULL) {
>   			return 0;
>   		}
>  -		ngroups = agroups;
>   		i = getgrouplist(user, primary, grouplist, &ngroups);
>  -		if ((i < 0) || (ngroups < 1)) {
>  -			agroups *= 2;
>  -			free(grouplist);
>  -		} else {
>  +		if (i >= 0) {
>   			for (i = 0; i < ngroups; i++) {
>   				if (grouplist[i] == target) {
>   					free(grouplist);
>   					return 1;
>   				}
>   			}
>  -			free(grouplist);
>   		}
>  -	} while (((i < 0) || (ngroups < 1)) && (agroups < 10000));
>  +		free(grouplist);
>  +	} while (i < 0 && ngroups > 0 && ngroups != pgroups &&
> ngroups <= NGROUPS_MAX);
>   	return 0;
>   }
>   #endif 
Yes, this looks much better than the unpatched edition.


-- 
ldv