Bug 98096 - is_invalid_shell with HAVE_GETUSERSHELL does nothing
Summary: is_invalid_shell with HAVE_GETUSERSHELL does nothing
Status: RESOLVED FIXED
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: medium trivial
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-05 13:34 UTC by Alexander Ried
Modified: 2016-10-05 18:33 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
logic fix (2.18 KB, patch)
2016-10-05 13:34 UTC, Alexander Ried
Details | Splinter Review
fix a typo in configure.ac (827 bytes, patch)
2016-10-05 16:17 UTC, Alexander Ried
Details | Splinter Review
fix logic if getusershell is present (1.23 KB, patch)
2016-10-05 16:34 UTC, Alexander Ried
Details | Splinter Review

Description Alexander Ried 2016-10-05 13:34:01 UTC
Created attachment 127028 [details] [review]
logic fix

The is_invalid_shell code in user-classify.c that is executed when HAVE_GETUSERSHELL is defined does nothing.
It only iterates and maybe sets ret = FALSE which is already set to FALSE anyway.


I have included a patch with what it should do in my opinion.
Comment 1 Ray Strode [halfline] 2016-10-05 15:35:59 UTC
Comment on attachment 127028 [details] [review]
logic fix

Review of attachment 127028 [details] [review]:
-----------------------------------------------------------------

Hey, thanks, for working on this! Clearly the existing code is wrong, but I think your fixes aren't quite right either.

::: configure.ac
@@ +215,4 @@
>  
>  AC_CHECK_LIB(c, getusershell, have_getusershell=yes, have_getusershell=no)
>  if test x$have_getusershell = xyes; then
> +  AC_DEFINE(HAVE_GETUSERSHELL, 1, [Define if getusershell() is available])

Ideally this would be a separate commit, but doesn't really matter.

::: src/user-classify.c
@@ +125,5 @@
>          setusershell ();
>          while ((valid_shell = getusershell ()) != NULL) {
> +                if (g_strcmp0 (shell, valid_shell) != 0) {
> +                        ret = TRUE;
> +                        break;

I think this needs to be g_strcmp0 (…) == 0 and ret = FALSE; to make logical sense.

@@ +131,3 @@
>          }
>          endusershell ();
> +#elif

we need to check /sbin/nologin and /bin/false explicitly.  In many distributions they aren't in /etc/shells, so the #elif isn't appropriate.

I think the best plan would be to leave the loop code untouched, and keep the int ret above the #ifdef and initialized to FALSE.  Just put ret = TRUE right before the setusershell() call after the #ifdef.
Comment 2 Alexander Ried 2016-10-05 16:14:53 UTC
(In reply to Ray Strode [halfline] from comment #1)
> ::: src/user-classify.c
> @@ +125,5 @@
> >          setusershell ();
> >          while ((valid_shell = getusershell ()) != NULL) {
> > +                if (g_strcmp0 (shell, valid_shell) != 0) {
> > +                        ret = TRUE;
> > +                        break;
> 
> I think this needs to be g_strcmp0 (…) == 0 and ret = FALSE; to make logical
> sense.
> 
You are absolutely right.

> @@ +131,3 @@
> >          }
> >          endusershell ();
> > +#elif
> 
> we need to check /sbin/nologin and /bin/false explicitly.  In many
> distributions they aren't in /etc/shells, so the #elif isn't appropriate.
> 
> I think the best plan would be to leave the loop code untouched, and keep
> the int ret above the #ifdef and initialized to FALSE.  Just put ret = TRUE
> right before the setusershell() call after the #ifdef.
You mean in many distros they _are_ included in /etc/shells? I just checked fedora and this is the case. I only checked on NixOS where they are not included in /etc/shells.

I'll send a new version of the patch, thanks for the quick feedback.
Comment 3 Alexander Ried 2016-10-05 16:17:20 UTC
Created attachment 127031 [details] [review]
fix a typo in configure.ac
Comment 4 Alexander Ried 2016-10-05 16:34:46 UTC
Created attachment 127032 [details] [review]
fix logic if getusershell is present
Comment 5 Ray Strode [halfline] 2016-10-05 18:32:00 UTC
(In reply to Alexander Ried from comment #2)
> I just checked
> fedora and this is the case. I only checked on NixOS where they are not
> included in /etc/shells.

Not for long:

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/UCUWTT63JS72R7ROFE46ZVUZLFN3K2MZ/
Comment 6 Ray Strode [halfline] 2016-10-05 18:33:56 UTC
pushed, thanks, much.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.