SELinux Archive on lore.kernel.org
 help / Atom feed
* [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
@ 2019-01-02 13:46 Laurent Bigonville
  2019-01-02 14:30 ` Laurent Bigonville
  0 siblings, 1 reply; 6+ messages in thread
From: Laurent Bigonville @ 2019-01-02 13:46 UTC (permalink / raw)
  To: selinux

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
  2019-01-02 13:46 [PATCH] libsemanage: Always set errno to 0 before calling getpwent() Laurent Bigonville
@ 2019-01-02 14:30 ` Laurent Bigonville
  2019-01-04 15:11   ` Stephen Smalley
  0 siblings, 1 reply; 6+ messages in thread
From: Laurent Bigonville @ 2019-01-02 14:30 UTC (permalink / raw)
  To: selinux

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,

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
  2019-01-02 14:30 ` Laurent Bigonville
@ 2019-01-04 15:11   ` Stephen Smalley
  2019-01-04 15:57     ` Laurent Bigonville
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2019-01-04 15:11 UTC (permalink / raw)
  To: Laurent Bigonville, selinux

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,


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
  2019-01-04 15:11   ` Stephen Smalley
@ 2019-01-04 15:57     ` Laurent Bigonville
  2019-01-04 16:09       ` Stephen Smalley
  0 siblings, 1 reply; 6+ messages in thread
From: Laurent Bigonville @ 2019-01-04 15:57 UTC (permalink / raw)
  To: Stephen Smalley, selinux

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,

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
  2019-01-04 15:57     ` Laurent Bigonville
@ 2019-01-04 16:09       ` Stephen Smalley
  2019-01-05 15:35         ` Nicolas Iooss
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2019-01-04 16:09 UTC (permalink / raw)
  To: Laurent Bigonville, selinux

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,


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libsemanage: Always set errno to 0 before calling getpwent()
  2019-01-04 16:09       ` Stephen Smalley
@ 2019-01-05 15:35         ` Nicolas Iooss
  0 siblings, 0 replies; 6+ messages in thread
From: Nicolas Iooss @ 2019-01-05 15:35 UTC (permalink / raw)
  To: Stephen Smalley, Laurent Bigonville; +Cc: selinux

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-02 13:46 [PATCH] libsemanage: Always set errno to 0 before calling getpwent() Laurent Bigonville
2019-01-02 14:30 ` Laurent Bigonville
2019-01-04 15:11   ` Stephen Smalley
2019-01-04 15:57     ` Laurent Bigonville
2019-01-04 16:09       ` Stephen Smalley
2019-01-05 15:35         ` Nicolas Iooss

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox