[musl] getusershell should ignore comments and empty lines.

[musl] getusershell should ignore comments and empty lines.

[Collin Funk]

Hello Musl maintainers,

The getusershell function behaves differently from Glibc and FreeBSD.
I believe that it should follow those implementations by ignoring
comments and empty lines.

I wrote a test for Gnulib that catches this issue. You may find it
helpful for testing [1].

On FreeBSD I have the following etc/shells:

==============================================
# List of acceptable shells for chpass(1).
# ftpd(8) will not allow users to connect who are not using
# one of these shells.



/bin/sh
/bin/csh
/bin/tcsh
/usr/local/bin/bash
/usr/local/bin/rbash
/usr/local/libexec/git-core/git-shell
==============================================

And I run the following in a Gnulib checkout:

  $ rm -rf testdir1 && ./gnulib-tool --create-testdir --dir testdir1 getusershell
  $ cd testdir1
  $ ./configure
  $ make
  $ ./gltests/test-getusershell
  /bin/sh
  /bin/csh
  /bin/tcsh
  /usr/local/bin/bash
  /usr/local/bin/rbash
  /usr/local/libexec/git-core/git-shell

GNU libc behaves the same way. I have not checked the other BSDs but I
assume they use the same code derived from 4.3BSD or 4.4BSD.

Using an Alpine Linux virtual machine with Musl Version 1.2.4_git20230717
and a few packages installed I have the default /etc/shells:

==============================================
# valid login shells
/bin/sh
/bin/ash
/bin/bash
==============================================

Using the same commands listed earlier I run:

   $ ./gltests/test-getusershell 
   test-getusershell.c:54: assertion 'ptr[0] != '#'' failed
   Aborted

And after adding an empty line before the comment:

  $ ./gltests/test-getusershell 
  test-getusershell.c:55: assertion 'ptr[0] != '\0'' failed
  Aborted

Let me know if you have any questions. The FreeBSD shells(5) man page
is pretty good and might be helpful [2]. Here is a link to their
implementation incase that helps too [3].

Collin

[1] https://git.savannah.gnu.org/cgit/gnulib.git/tree/tests/test-getusershell.c
[2] https://man.freebsd.org/cgi/man.cgi?query=shells&sektion=5&manpath=freebsd-release
[3] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/gen/getusershell.c

Re: [musl] getusershell should ignore comments and empty lines.

[Rich Felker]

On Fri, May 17, 2024 at 08:17:54PM -0700, Collin Funk wrote:
> Hello Musl maintainers,
> 
> The getusershell function behaves differently from Glibc and FreeBSD.
> I believe that it should follow those implementations by ignoring
> comments and empty lines.
> 
> I wrote a test for Gnulib that catches this issue. You may find it
> helpful for testing [1].
> 
> On FreeBSD I have the following etc/shells:
> 
> ==============================================
> # List of acceptable shells for chpass(1).
> # ftpd(8) will not allow users to connect who are not using
> # one of these shells.
> 
> 
> 
> /bin/sh
> /bin/csh
> /bin/tcsh
> /usr/local/bin/bash
> /usr/local/bin/rbash
> /usr/local/libexec/git-core/git-shell
> ==============================================
> 
> And I run the following in a Gnulib checkout:
> 
>   $ rm -rf testdir1 && ./gnulib-tool --create-testdir --dir testdir1 getusershell
>   $ cd testdir1
>   $ ./configure
>   $ make
>   $ ./gltests/test-getusershell
>   /bin/sh
>   /bin/csh
>   /bin/tcsh
>   /usr/local/bin/bash
>   /usr/local/bin/rbash
>   /usr/local/libexec/git-core/git-shell
> 
> GNU libc behaves the same way. I have not checked the other BSDs but I
> assume they use the same code derived from 4.3BSD or 4.4BSD.
> 
> Using an Alpine Linux virtual machine with Musl Version 1.2.4_git20230717
> and a few packages installed I have the default /etc/shells:
> 
> ==============================================
> # valid login shells
> /bin/sh
> /bin/ash
> /bin/bash
> ==============================================
> 
> Using the same commands listed earlier I run:
> 
>    $ ./gltests/test-getusershell 
>    test-getusershell.c:54: assertion 'ptr[0] != '#'' failed
>    Aborted
> 
> And after adding an empty line before the comment:
> 
>   $ ./gltests/test-getusershell 
>   test-getusershell.c:55: assertion 'ptr[0] != '\0'' failed
>   Aborted
> 
> Let me know if you have any questions. The FreeBSD shells(5) man page
> is pretty good and might be helpful [2]. Here is a link to their
> implementation incase that helps too [3].

It says:

    "A hash mark (``#'') indicates the beginning of a comment;
    subsequent characters up to the end of the line are not
    interpreted by the routines which search the file."

This isn't very clear whether # is only a comment on the beginning of
a line (after potential whitespace?) or whether # appearing in a line
with a shell pathname is a comment or part of the pathname. If it's a
comment, it's not clear if whitespace before it is part of the shell
pathname -- e.g. does "/bin/sh # best shell" define "/bin/sh" or
"/bin/sh " as the shell pathname?

It sounds like nobody ever thought about whitespace, quoting, or
rigorous comment syntax here...

Rich

Re: [musl] getusershell should ignore comments and empty lines.

[Leah Neukirchen]

Rich Felker <dalias@libc.org> writes:

> It says:
>
>     "A hash mark (``#'') indicates the beginning of a comment;
>     subsequent characters up to the end of the line are not
>     interpreted by the routines which search the file."
>
> This isn't very clear whether # is only a comment on the beginning of
> a line (after potential whitespace?) or whether # appearing in a line
> with a shell pathname is a comment or part of the pathname. If it's a
> comment, it's not clear if whitespace before it is part of the shell
> pathname -- e.g. does "/bin/sh # best shell" define "/bin/sh" or
> "/bin/sh " as the shell pathname?
>
> It sounds like nobody ever thought about whitespace, quoting, or
> rigorous comment syntax here...

True:

OpenBSD drops the rest of the line with "#" and ignores lines not
starting with a "/".

glibc drops the rest of the line with "#", elides spaces after the
entry, and skips everything before the first "/" (quite bold).

pam_shells skips all lines that don't start with a "/" and doesn't
handle "#" specially.

gnome-terminal just tries to find one line that matches exactly the
the shell

util-linux skips lines from getusershell that start with "#".
Likewise "seunshare".

-- 
Leah Neukirchen  <leah@vuxu.org>  https://leahneukirchen.org/

Re: [musl] getusershell should ignore comments and empty lines.

[Collin Funk]

Hi Rich and Leah,

On 5/23/24 6:45 AM, Leah Neukirchen wrote:
>>     "A hash mark (``#'') indicates the beginning of a comment;
>>     subsequent characters up to the end of the line are not
>>     interpreted by the routines which search the file."
>>
>> This isn't very clear whether # is only a comment on the beginning of
>> a line (after potential whitespace?) or whether # appearing in a line
>> with a shell pathname is a comment or part of the pathname. If it's a
>> comment, it's not clear if whitespace before it is part of the shell
>> pathname -- e.g. does "/bin/sh # best shell" define "/bin/sh" or
>> "/bin/sh " as the shell pathname?
>>
>> It sounds like nobody ever thought about whitespace, quoting, or
>> rigorous comment syntax here...
>
> True:
> 
> OpenBSD drops the rest of the line with "#" and ignores lines not
> starting with a "/".
> 
> glibc drops the rest of the line with "#", elides spaces after the
> entry, and skips everything before the first "/" (quite bold).

I noticed the glibc behavior as well, but I thought the BSDs did it
too. Like this:

     'bin/bash' -> '/bash'

which is interesting. :)

I think the general algorithm is read a single line. Find the first
'/' character and then begin at the next character. Then continue
until the end of line or other whitespace character. Perhaps the
others felt that was awkward and changed it, I am not sure.

I think it would be nice to ignore lines starting with '#' though and
empty lines. That should cover 99% of cases. Most distributions,
including Alpine Linux, come with an '/etc/shells' like this:

===================
# Some commentary redirecting to a man page or other documentation.

/bin/sh
/bin/bash
/usr/bin/sh
/usr/bin/bash
===================

I doubt anyone changes it from that simple format.

Collin

Re: [musl] getusershell should ignore comments and empty lines.

[Rich Felker]

On Thu, May 23, 2024 at 03:45:10PM +0200, Leah Neukirchen wrote:
> Rich Felker <dalias@libc.org> writes:
> 
> > It says:
> >
> >     "A hash mark (``#'') indicates the beginning of a comment;
> >     subsequent characters up to the end of the line are not
> >     interpreted by the routines which search the file."
> >
> > This isn't very clear whether # is only a comment on the beginning of
> > a line (after potential whitespace?) or whether # appearing in a line
> > with a shell pathname is a comment or part of the pathname. If it's a
> > comment, it's not clear if whitespace before it is part of the shell
> > pathname -- e.g. does "/bin/sh # best shell" define "/bin/sh" or
> > "/bin/sh " as the shell pathname?
> >
> > It sounds like nobody ever thought about whitespace, quoting, or
> > rigorous comment syntax here...
> 
> True:
> 
> OpenBSD drops the rest of the line with "#" and ignores lines not
> starting with a "/".
> 
> glibc drops the rest of the line with "#", elides spaces after the
> entry, and skips everything before the first "/" (quite bold).
> 
> pam_shells skips all lines that don't start with a "/" and doesn't
> handle "#" specially.
> 
> gnome-terminal just tries to find one line that matches exactly the
> the shell
> 
> util-linux skips lines from getusershell that start with "#".
> Likewise "seunshare".

In general, musl policy is to support at most what's consistent
between historical implementations.

It seems like they all ignore lines that start with a '#'. They might
(not clear from the above) also ignore blank lines.

The minimal change both to avoid clashing with vaguely reasonable
things user might want to put there (and violating least surprise),
and to support the common behavior, seems to be just looping as long
as the line is empty or starts with #.

Rich

Re: [musl] getusershell should ignore comments and empty lines.

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

On Thu, May 23, 2024 at 10:47:44AM -0400, Rich Felker wrote:
> On Thu, May 23, 2024 at 03:45:10PM +0200, Leah Neukirchen wrote:
> > Rich Felker <dalias@libc.org> writes:
> > 
> > > It says:
> > >
> > >     "A hash mark (``#'') indicates the beginning of a comment;
> > >     subsequent characters up to the end of the line are not
> > >     interpreted by the routines which search the file."
> > >
> > > This isn't very clear whether # is only a comment on the beginning of
> > > a line (after potential whitespace?) or whether # appearing in a line
> > > with a shell pathname is a comment or part of the pathname. If it's a
> > > comment, it's not clear if whitespace before it is part of the shell
> > > pathname -- e.g. does "/bin/sh # best shell" define "/bin/sh" or
> > > "/bin/sh " as the shell pathname?
> > >
> > > It sounds like nobody ever thought about whitespace, quoting, or
> > > rigorous comment syntax here...
> > 
> > True:
> > 
> > OpenBSD drops the rest of the line with "#" and ignores lines not
> > starting with a "/".
> > 
> > glibc drops the rest of the line with "#", elides spaces after the
> > entry, and skips everything before the first "/" (quite bold).
> > 
> > pam_shells skips all lines that don't start with a "/" and doesn't
> > handle "#" specially.
> > 
> > gnome-terminal just tries to find one line that matches exactly the
> > the shell
> > 
> > util-linux skips lines from getusershell that start with "#".
> > Likewise "seunshare".
> 
> In general, musl policy is to support at most what's consistent
> between historical implementations.
> 
> It seems like they all ignore lines that start with a '#'. They might
> (not clear from the above) also ignore blank lines.
> 
> The minimal change both to avoid clashing with vaguely reasonable
> things user might want to put there (and violating least surprise),
> and to support the common behavior, seems to be just looping as long
> as the line is empty or starts with #.

Something like this?

Rich

[-- Attachment #2: usershell.diff --]
[-- Type: text/plain, Size: 496 bytes --]

diff --git a/src/legacy/getusershell.c b/src/legacy/getusershell.c
index 5fecdec2..1c5d98ec 100644
--- a/src/legacy/getusershell.c
+++ b/src/legacy/getusershell.c
@@ -25,8 +25,10 @@ char *getusershell(void)
 	ssize_t l;
 	if (!f) setusershell();
 	if (!f) return 0;
-	l = getline(&line, &linesize, f);
-	if (l <= 0) return 0;
+	do {
+		l = getline(&line, &linesize, f);
+		if (l <= 0) return 0;
+	} while (line[0] == '#' || line[0] == '\n');
 	if (line[l-1]=='\n') line[l-1]=0;
 	return line;
 }

Re: [musl] getusershell should ignore comments and empty lines.

[Collin Funk]

Hi Rich,

On 5/23/24 8:27 AM, Rich Felker wrote:
>> In general, musl policy is to support at most what's consistent
>> between historical implementations.
>>
>> It seems like they all ignore lines that start with a '#'. They might
>> (not clear from the above) also ignore blank lines.
>>
>> The minimal change both to avoid clashing with vaguely reasonable
>> things user might want to put there (and violating least surprise),
>> and to support the common behavior, seems to be just looping as long
>> as the line is empty or starts with #.
>
> Something like this?

Thanks for explaining how Musl handles things.

I haven't tested your patch but it looks correct.

Collin

Re: [musl] getusershell should ignore comments and empty lines.

[Rich Felker]

On Fri, May 24, 2024 at 05:16:41PM -0700, Collin Funk wrote:
> Hi Rich,
> 
> On 5/23/24 8:27 AM, Rich Felker wrote:
> >> In general, musl policy is to support at most what's consistent
> >> between historical implementations.
> >>
> >> It seems like they all ignore lines that start with a '#'. They might
> >> (not clear from the above) also ignore blank lines.
> >>
> >> The minimal change both to avoid clashing with vaguely reasonable
> >> things user might want to put there (and violating least surprise),
> >> and to support the common behavior, seems to be just looping as long
> >> as the line is empty or starts with #.
> >
> > Something like this?
> 
> Thanks for explaining how Musl handles things.
> 
> I haven't tested your patch but it looks correct.

Applied. Let me know if anything seems wrong still.