Re: [PATCH] getusershell: Work around musl bugs.

[Bruno Haible]

Hi Collin,

> >> It looks like the current code wants drive-prefixes accepted, i.e.
> >> 'c:/ugly/windows/stuff'.
> > 
> > ?? I don't see such code in gnulib/lib/getusershell.c.
> 
> It is defined in the 'ADDITIONAL_DEFAULT_SHELLS' macro [1]:
> 
> 
> #if ! defined ADDITIONAL_DEFAULT_SHELLS && defined __MSDOS__
> # define ADDITIONAL_DEFAULT_SHELLS \
>   "c:/dos/command.com", "c:/windows/command.com", "c:/command.com",

Inside '#if defined __MSDOS__' or '#if defined _WIN32' this is OK.
I was worried there was explicit code to accept such prefixes on POSIX
compatible platforms. Fortunately not.

> I was trying to point out that glibc and BSD will only return absolute
> file names. Since they do not target Windows that means just looking
> for a '/'.

Ah, yes, the glibc implementation tests for a '/' somewhere in the loop.

> =
> 
> this-part-gets-removed/this-doesn't/bash
> /bin/sh
> =
> 
> And then the program output:
> 
> $ uname -o
> GNU/Linux
> $ ./a.out 
> /this-doesn't/bash
> /bin/sh
> 
> The file format isn't very well defined, in my opinion [3]. I think
> the common assumption is that administrators put sane absolute file
> names one-per-line.

Yes, stripping off the first component of a relative file name is
certainly not what anyone would expect. Your choice of dropping that
string entirely is better, IMO.

Your use of IS_ABSOLUTE_FILE_NAME is fine.

Bruno

Re: [PATCH] getusershell: Work around musl bugs.

[Collin Funk]

Hi Bruno,

On 5/20/24 3:28 AM, Bruno Haible wrote:
>> It looks like the current code wants drive-prefixes accepted, i.e.
>> 'c:/ugly/windows/stuff'.
> 
> ?? I don't see such code in gnulib/lib/getusershell.c.

It is defined in the 'ADDITIONAL_DEFAULT_SHELLS' macro [1]:


#if ! defined ADDITIONAL_DEFAULT_SHELLS && defined __MSDOS__
# define ADDITIONAL_DEFAULT_SHELLS \
  "c:/dos/command.com", "c:/windows/command.com", "c:/command.com",
#else
# define ADDITIONAL_DEFAULT_SHELLS /* empty */
#endif

/* List of shells to use if the shells file is missing. */
static char const* const default_shells[] =
{
  ADDITIONAL_DEFAULT_SHELLS
  "/bin/sh", "/bin/csh", "/usr/bin/sh", "/usr/bin/csh", NULL
};


>> That sort of breaks the behavior of glibc and BSD where:
>>
>>  input -> getusershell () output
>>  'bin/bash' -> '/bash'
> 
> ?? The code in gnulib/lib/getusershell.c does not test for a '/'.

I probably explained it poorly, sorry. Perhaps the important section
of code in glibc will help explain [2].

I was trying to point out that glibc and BSD will only return absolute
file names. Since they do not target Windows that means just looking
for a '/'.

Here is a test program:

=
#define _GNU_SOURCE 1
#include 
#include 
#include 
int
main (void)
{
  char *p;
  while ((p = getusershell ()))
printf ("%s\n", p);
  return 0;
}
=

My /etc/shells:

=

this-part-gets-removed/this-doesn't/bash
/bin/sh
=

And then the program output:

$ uname -o
GNU/Linux
$ ./a.out 
/this-doesn't/bash
/bin/sh

The file format isn't very well defined, in my opinion [3]. I think
the common assumption is that administrators put sane absolute file
names one-per-line. Like this:

/bin/bash
/bin/sh
/bin/ksh
/usr/bin/csh

The other patch I sent would behave similarly enough to glibc and BSD
in my opinion while allowing for drive prefixes on Windows and spaces
since I think they are common there.

The removal of the first non-absolute part is strange in my opinion.

Collin

[1] https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/getusershell.c#n48
[2] 
https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/getusershell.c;h=4221095dca743dfa5067637f1aad4651bd5cf279;hb=2be3352f0b1ebaa39596393fffe1062275186669#l130
[3] 
https://man.freebsd.org/cgi/man.cgi?query=shells=5=0=FreeBSD+14.0-RELEASE+and+Ports

Re: [PATCH] getusershell: Work around musl bugs.

[Bruno Haible]

Collin Funk wrote:
> It looks like the current code wants drive-prefixes accepted, i.e.
> 'c:/ugly/windows/stuff'.

?? I don't see such code in gnulib/lib/getusershell.c.

> That sort of breaks the behavior of glibc and BSD where:
> 
>  input -> getusershell () output
>  'bin/bash' -> '/bash'

?? The code in gnulib/lib/getusershell.c does not test for a '/'.

Bruno

Re: [PATCH] getusershell: Work around musl bugs.

[Collin Funk]

On 5/19/24 7:17 PM, Bruno Haible wrote:
> You don't need an Alpine Linux machine to debug this. You can debug it
> on a glibc system like this:
>   1. create a testdir for the module getusershell,
>   2. configure through
>ac_cv_func_getusershell=no ./configure
>   3. use the unit test, or compile a test program with -Itestdir/gllib
>  -Itestdir and link it with testdir/gllib/libgnu.a.

Ah, I forgot about this thanks.

It looks like the current code wants drive-prefixes accepted, i.e.
'c:/ugly/windows/stuff'.

That sort of breaks the behavior of glibc and BSD where:

 input -> getusershell () output
 'bin/bash' -> '/bash'

Since it seems silly to accept some drive prefix in the middle of a
string.

I can't imagine anyone putting non-absolute file names in that file.
So I'll probably restrict it to one absolute file name per-line and
then trim any leading whitespace plus comments. Perhaps getline and
filename.h macros will help there.

Collin

Re: [PATCH] getusershell: Work around musl bugs.

[Bruno Haible]

Collin Funk wrote:
> Sadly, it looks like this uncovered that the gnulib version is buggy
> too.

You don't need an Alpine Linux machine to debug this. You can debug it
on a glibc system like this:
  1. create a testdir for the module getusershell,
  2. configure through
   ac_cv_func_getusershell=no ./configure
  3. use the unit test, or compile a test program with -Itestdir/gllib
 -Itestdir and link it with testdir/gllib/libgnu.a.

Bruno

Re: [PATCH] getusershell: Work around musl bugs.

[Collin Funk]

Hi Bruno,

On 5/19/24 5:03 PM, Bruno Haible wrote:
> Perfect. Thanks a lot!

Sadly, it looks like this uncovered that the gnulib version is buggy
too.

Given this etc/shells:


# valid login shells

/bin/sh  # abc
/bin/ash # 123 
/bin/bash  # def



$ ./gltests/test-getusershell 
valid
login
shells
/bin/sh
abc
/bin/ash
123
/bin/bash
def
abc
/bin/sh
abc

I'll have a look at fixing that now.

Collin

Re: [PATCH] getusershell: Work around musl bugs.

[Bruno Haible]

Collin Funk wrote:
> + getusershell: Work around musl bugs.
> + Reported by Bruno Haible in
> + .
> + * doc/glibc-functions/getusershell.texi: Mention the musl bug.
> + * lib/unistd.in.h (getusershell, setusershell, endusershell): Allow the
> + functions to be declared with the rpl_ prefix.
> + * m4/getusershell.m4 (gl_FUNC_GETUSERSHELL): Prepare functions to be
> + replaced on musl systems.
> + (gl_PREREQ_GETUSERSHELL): New macro.
> + * m4/unistd_h.m4 (gl_UNISTD_H_DEFAULTS): Initialize
> + REPLACE_GETUSERSHELL.
> + * modules/getusershell (Depends-on): Update module conditions to account
> + for the function being available but replaced by Gnulib.
> + (configure.ac): Likewise. Invoke gl_PREREQ_GETUSERSHELL.

Perfect. Thanks a lot!

Bruno