Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()

[Laurent Bigonville <bigon@debian.org>]

Le 4/01/19 à 16:11, Stephen Smalley a écrit :
> On Wed, 2019-01-02 at 15:30 +0100, Laurent Bigonville wrote:
>> Le 2/01/19 à 14:46, Laurent Bigonville a écrit :
>>> From: Laurent Bigonville <bigon@bigon.be>
>>>
>>> The manpage explicitly states that:
>>>
>>>     The  getpwent()  function  returns a pointer to a passwd
>>> structure, or
>>>     NULL if there are no more entries or an error occurred.  If an
>>> error
>>>     occurs, errno is set appropriately.  If one wants to check errno
>>> after
>>>     the call, it should be set to zero before the call.
>>>
>>> Without this, genhomedircon can wrongly return the following:
>>>     libsemanage.get_home_dirs: Error while fetching
>>> users.  Returning list so far.
>>>
>>> https://github.com/SELinuxProject/selinux/issues/121
>>>
>>> Signed-off-by: Laurent Bigonville <bigon@bigon.be>
>>> ---
>>>    libsemanage/src/genhomedircon.c | 13 ++++++++++---
>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libsemanage/src/genhomedircon.c
>>> b/libsemanage/src/genhomedircon.c
>>> index 3e61b510..591941fb 100644
>>> --- a/libsemanage/src/genhomedircon.c
>>> +++ b/libsemanage/src/genhomedircon.c
>>> @@ -361,7 +361,11 @@ static semanage_list_t
>>> *get_home_dirs(genhomedircon_settings_t * s)
>>>    
>>>    	errno = 0;
>>>    	setpwent();
>>> -	while ((pwbuf = getpwent()) != NULL) {
>>> +	while (1) {
>>> +		errno = 0;
>>> +		pwbuf = getpwent();
>>> +		if (pwbuf == NULL)
>>> +			break;
>>>    		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
>>>    			continue;
>>>    		if (!semanage_list_find(shells, pwbuf->pw_shell))
>>> @@ -403,7 +407,6 @@ static semanage_list_t
>>> *get_home_dirs(genhomedircon_settings_t * s)
>>>    		}
>>>    		free(path);
>>>    		path = NULL;
>>> -		errno = 0;
>> Actually I'm wondering if this shouldn't stay there
> Why?

According to strdup() manpage, it can also change errno, isn't there a 
risk that errno is changed to an non-zero value and then causing the 
warning below to be printed?


>
>>>    	}
>>>    
>>>    	if (errno) {
>>> @@ -1101,7 +1104,11 @@ static int
>>> get_group_users(genhomedircon_settings_t * s,
>>>    	}
>>>    
>>>    	setpwent();
>>> -	while ((pw = getpwent()) != NULL) {
>>> +	while (1) {
>>> +		errno = 0;
>>> +		pw = getpwent();
>>> +		if (pw == NULL)
>>> +			break;
>>>    		// skip users who also have this group as their
>>>    		// primary group
>>>    		if (lfind(pw->pw_name, group->gr_mem, &nmembers,