diff mbox series

libsemanage: Always set errno to 0 before calling getpwent()

Message ID 20190102134639.30515-1-bigon@debian.org (mailing list archive)
State Not Applicable
Headers show
Series libsemanage: Always set errno to 0 before calling getpwent() | expand

Commit Message

Laurent Bigonville Jan. 2, 2019, 1:46 p.m. UTC 
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(-)

Comments

Laurent Bigonville Jan. 2, 2019, 2:30 p.m. UTC | #1 
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

>   	}
>   
>   	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,
Stephen Smalley Jan. 4, 2019, 3:11 p.m. UTC | #2 
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?

> 
> >   	}
> >   
> >   	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,
Laurent Bigonville Jan. 4, 2019, 3:57 p.m. UTC | #3 
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,
Stephen Smalley Jan. 4, 2019, 4:09 p.m. UTC | #4 
On Fri, 2019-01-04 at 16:57 +0100, Laurent Bigonville wrote:
> 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?

On strdup(pwbuf->pw_dir) failure, we'll break out of the loop with non-
zero errno and then correctly report that there was an error that
truncated the list.  That's correct behavior.  The errno = 0; were are
removing is within the loop and is obsoleted by your setting of it at
the beginning of the loop.  So I think your patch is fine as is,
although I have not tested it.

> 
> 
> > > >    	}
> > > >    
> > > >    	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,
Nicolas Iooss Jan. 5, 2019, 3:35 p.m. UTC | #5 
On Fri, Jan 4, 2019 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On Fri, 2019-01-04 at 16:57 +0100, Laurent Bigonville wrote:
> > 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?
>
> On strdup(pwbuf->pw_dir) failure, we'll break out of the loop with non-
> zero errno and then correctly report that there was an error that
> truncated the list.  That's correct behavior.  The errno = 0; were are
> removing is within the loop and is obsoleted by your setting of it at
> the beginning of the loop.  So I think your patch is fine as is,
> although I have not tested it.

I have tested it on my test system. Before applying the patch,
"semanage fcontext -m ..." displayed "libsemanage.get_home_dirs: Error
while fetching users.  Returning list so far.". This message
disappears as expected with the patch.

Merged. Thanks!
Nicolas
>
> >
> >
> > > > >         }
> > > > >
> > > > >         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,
>
diff mbox series

Patch

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;
 	}
 
 	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,