Patchwork getusershell: rewrite from scratch [BZ #18660]

login
register
mail settings
Submitter Mike Frysinger
Date Oct. 21, 2015, 5:39 a.m.
Message ID <1445405944-19916-1-git-send-email-vapier@gentoo.org>
Download mbox | patch
Permalink /patch/9284/
State Under Review
Delegated to: Mike Frysinger
Headers show

Comments

Mike Frysinger - Oct. 21, 2015, 5:39 a.m.
The current getusershell implementation is ancient, buggy, and overly
complicated.  Rewrite it from scratch and exercise all the demons.

This does change the API a bit: the old one basically did:
- skip all leading bytes until a '#' or '/' is found
- if end of line, skip the line
- if byte is '#', skip the line
- if byte is '/' and is the end of file, skip the line
- if byte is '/', scan all bytes until [[:space:]#\0] is hit, set that
  byte to NUL, and return the line
This is pretty idiosyncratic behavior.  Some examples:
  " /foo bar" -> "/foo"
  " /fat#cat" -> "/fat"
  "space/cow" -> "/cow"
  "/"         -> "/"    (unless this is the last byte in the file!)

The new one is much simpler: if the first byte is a '/', then return
the line as-is.  Everything else is ignored.

Comparing the API to other projects:
- gnulib returns any line that does not start with '#'
- FreeBSD behaves same as glibc before this, except it allows / at the end
- OpenBSD behaves the same as FreeBSD
- Darwin behaves the same as FreeBSD
- shadow's chsh ignores lines that start with '#' but allows all others
- util-linux's chsh ignores lines that start with '#', is just "", or if
  the last byte of the file is '/', but allows all others

Comparing it to documentation of getusershell(3):
- glibc doesn't document this function
- Linux man-pages just says "The line should contain the pathname of a
  valid user shell."
- gnulib says "Return names of valid user shells."
- FreeBSD man says "... returns a pointer to a valid user shell as defined
  by the system manager in the shells database as described in shells(5)."
- OpenBSD man says "... returns a pointer to a legal user shell as defined
  by the system manager in the file /etc/shells."
- Darwin says the same as OpenBSD

And to the shells(5) documentation:
- Linux man-pages says "... a text file which contains the full pathnames
  of valid login shells."
- FreeBSD says "The shells file contains a list of the shells on the
  system.  For each shell a single line should be present, consisting of
  the shell's path, relative to root.  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.  Blank lines
  are also ignored."
- OpenBSD says pretty much the same thing as FreeBSD
- Darwin says the same as OpenBSD

And to the chsh(1) documentation:
- shadow says "... login shell is that the command name must be listed in
  /etc/shells, ..." and "/etc/shells: List of valid login shells"
- util-linux says "chsh will accept the full pathname of any executable
  file on the system.  However, it will issue a warning if the shell is
  not listed in the /etc/shells file."

I think the new glibc behavior matches the intentions of these APIs while
producing small code.  Namely, getusershell is supposed to return "valid"
shells from /etc/shells, and people define that as absolute paths to an
interactive program.  Relative paths are dangerous and undesirable, as are
random lines w/out any paths at all.  Arbitrarily splitting lines on space
and hash marks is also dangerous/surprising.

2015-10-19  Mike Frysinger  <vapier@gentoo.org>

	[BZ #18660]
	* misc/Makefile (tests): Add tst-getusershell and tst-endusershell.
	* misc/getusershell.c: Rewrite from scratch.
	* misc/tst-getusershell.c: New test.
	* misc/tst-endusershell.c: Likewise.
---
 NEWS                    |   4 +
 misc/Makefile           |   3 +-
 misc/getusershell.c     | 213 +++++++++++++++++++++---------------------------
 misc/tst-endusershell.c |  64 +++++++++++++++
 misc/tst-getusershell.c | 178 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 339 insertions(+), 123 deletions(-)
 create mode 100644 misc/tst-endusershell.c
 create mode 100644 misc/tst-getusershell.c
Andreas Schwab - Oct. 21, 2015, 7:47 a.m.
Mike Frysinger <vapier@gentoo.org> writes:

> - FreeBSD says "The shells file contains a list of the shells on the
>   system.  For each shell a single line should be present, consisting of
>   the shell's path, relative to root.  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.  Blank lines
>   are also ignored."

This is the ultimate documentation, as the interface was invented by
BSD.  Why deviate from it?

Andreas.
Joseph Myers - Oct. 21, 2015, 12:02 p.m.
On Wed, 21 Oct 2015, Mike Frysinger wrote:

> +/* Copyright (C) 2015 Free Software Foundation, Inc.

The first line of any new file should be a descriptive comment, before the 
copyright notice.
Mike Frysinger - Oct. 21, 2015, 2:43 p.m.
On 21 Oct 2015 09:47, Andreas Schwab wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> > - FreeBSD says "The shells file contains a list of the shells on the
> >   system.  For each shell a single line should be present, consisting of
> >   the shell's path, relative to root.  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.  Blank lines
> >   are also ignored."
> 
> This is the ultimate documentation, as the interface was invented by
> BSD.  Why deviate from it?

i disagree.  as i've shown, there is deviation between systems already,
which means admins cannot rely on the behavior being the same everywhere.
hence i turn to the intention of the API and how a reasonable admin would
configure the system, and i think this new version follows that.

note that the API/ABI for tools is unchanged here -- programs all continue
to run and behave the same.  it's changing how the contents of the shells
file is parsed and what entries are returned.
-mike
Tolga Dalman - Nov. 3, 2015, 12:39 a.m.
Mike,

I like this implementation as it really an improvement to the status quo (IMHO). Also, the test cases are a great benefit as well :)

Best regards
Tolga Dalman

On 10/21/2015 07:39 AM, Mike Frysinger wrote:
> The current getusershell implementation is ancient, buggy, and overly
> complicated.  Rewrite it from scratch and exercise all the demons.
> 
> This does change the API a bit: the old one basically did:
> - skip all leading bytes until a '#' or '/' is found
> - if end of line, skip the line
> - if byte is '#', skip the line
> - if byte is '/' and is the end of file, skip the line
> - if byte is '/', scan all bytes until [[:space:]#\0] is hit, set that
>   byte to NUL, and return the line
> This is pretty idiosyncratic behavior.  Some examples:
>   " /foo bar" -> "/foo"
>   " /fat#cat" -> "/fat"
>   "space/cow" -> "/cow"
>   "/"         -> "/"    (unless this is the last byte in the file!)
> 
> The new one is much simpler: if the first byte is a '/', then return
> the line as-is.  Everything else is ignored.
> 
> Comparing the API to other projects:
> - gnulib returns any line that does not start with '#'
> - FreeBSD behaves same as glibc before this, except it allows / at the end
> - OpenBSD behaves the same as FreeBSD
> - Darwin behaves the same as FreeBSD
> - shadow's chsh ignores lines that start with '#' but allows all others
> - util-linux's chsh ignores lines that start with '#', is just "", or if
>   the last byte of the file is '/', but allows all others
> 
> Comparing it to documentation of getusershell(3):
> - glibc doesn't document this function
> - Linux man-pages just says "The line should contain the pathname of a
>   valid user shell."
> - gnulib says "Return names of valid user shells."
> - FreeBSD man says "... returns a pointer to a valid user shell as defined
>   by the system manager in the shells database as described in shells(5)."
> - OpenBSD man says "... returns a pointer to a legal user shell as defined
>   by the system manager in the file /etc/shells."
> - Darwin says the same as OpenBSD
> 
> And to the shells(5) documentation:
> - Linux man-pages says "... a text file which contains the full pathnames
>   of valid login shells."
> - FreeBSD says "The shells file contains a list of the shells on the
>   system.  For each shell a single line should be present, consisting of
>   the shell's path, relative to root.  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.  Blank lines
>   are also ignored."
> - OpenBSD says pretty much the same thing as FreeBSD
> - Darwin says the same as OpenBSD
> 
> And to the chsh(1) documentation:
> - shadow says "... login shell is that the command name must be listed in
>   /etc/shells, ..." and "/etc/shells: List of valid login shells"
> - util-linux says "chsh will accept the full pathname of any executable
>   file on the system.  However, it will issue a warning if the shell is
>   not listed in the /etc/shells file."
> 
> I think the new glibc behavior matches the intentions of these APIs while
> producing small code.  Namely, getusershell is supposed to return "valid"
> shells from /etc/shells, and people define that as absolute paths to an
> interactive program.  Relative paths are dangerous and undesirable, as are
> random lines w/out any paths at all.  Arbitrarily splitting lines on space
> and hash marks is also dangerous/surprising.
> 
> 2015-10-19  Mike Frysinger  <vapier@gentoo.org>
> 
> 	[BZ #18660]
> 	* misc/Makefile (tests): Add tst-getusershell and tst-endusershell.
> 	* misc/getusershell.c: Rewrite from scratch.
> 	* misc/tst-getusershell.c: New test.
> 	* misc/tst-endusershell.c: Likewise.
> ---
>  NEWS                    |   4 +
>  misc/Makefile           |   3 +-
>  misc/getusershell.c     | 213 +++++++++++++++++++++---------------------------
>  misc/tst-endusershell.c |  64 +++++++++++++++
>  misc/tst-getusershell.c | 178 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 339 insertions(+), 123 deletions(-)
>  create mode 100644 misc/tst-endusershell.c
>  create mode 100644 misc/tst-getusershell.c
> 
> diff --git a/NEWS b/NEWS
> index 1313ab2..4ba05e2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -22,6 +22,10 @@ Version 2.23
>    19049, 19050, 19059, 19071, 19074, 19076, 19077, 19078, 19079, 19085,
>    19086, 19088, 19094, 19095, 19124, 19125, 19129, 19134, 19137.
>  
> +* The getusershell function now returns entries from /etc/shells only when
> +  the line starts with a '/', and no longer chops lines based on '#' or white
> +  space.
> +
>  * There is now a --disable-timezone-tools configure option for disabling the
>    building and installing of the timezone related utilities (zic, zdump, and
>    tzselect).  This is useful for people who build the timezone data and code
> diff --git a/misc/Makefile b/misc/Makefile
> index 2f5edf6..3fc44f1 100644
> --- a/misc/Makefile
> +++ b/misc/Makefile
> @@ -77,7 +77,8 @@ gpl2lgpl := error.c error.h
>  
>  tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
>  	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
> -	 tst-mntent-blank-corrupt tst-mntent-blank-passno
> +	 tst-mntent-blank-corrupt tst-mntent-blank-passno tst-getusershell \
> +	 tst-endusershell
>  ifeq ($(run-built-tests),yes)
>  tests-special += $(objpfx)tst-error1-mem.out
>  endif
> diff --git a/misc/getusershell.c b/misc/getusershell.c
> index fc2c43b..3d3f221 100644
> --- a/misc/getusershell.c
> +++ b/misc/getusershell.c
> @@ -1,143 +1,112 @@
> -/*
> - * Copyright (c) 1985, 1993
> - *	The Regents of the University of California.  All rights reserved.
> - *
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions
> - * are met:
> - * 1. Redistributions of source code must retain the above copyright
> - *    notice, this list of conditions and the following disclaimer.
> - * 2. Redistributions in binary form must reproduce the above copyright
> - *    notice, this list of conditions and the following disclaimer in the
> - *    documentation and/or other materials provided with the distribution.
> - * 4. Neither the name of the University nor the names of its contributors
> - *    may be used to endorse or promote products derived from this software
> - *    without specific prior written permission.
> - *
> - * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
> - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> - * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
> - * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> - * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> - * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> - * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> - * SUCH DAMAGE.
> - */
> -
> -#if defined(LIBC_SCCS) && !defined(lint)
> -static char sccsid[] = "@(#)getusershell.c	8.1 (Berkeley) 6/4/93";
> -#endif /* LIBC_SCCS and not lint */
> -
> -#include <sys/param.h>
> -#include <sys/file.h>
> -#include <sys/stat.h>
> +/* Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <assert.h>
> +#include <paths.h>
>  #include <stdio.h>
> -#include <stdio_ext.h>
> -#include <ctype.h>
>  #include <stdlib.h>
> +#include <string.h>
>  #include <unistd.h>
> -#include <paths.h>
>  
> -/*
> - * Local shells should NOT be added here.  They should be added in
> - * /etc/shells.
> - */
> -
> -/* NB: we do not initialize okshells here.  The initialization needs
> -   relocations.  These interfaces are used so rarely that this is not
> -   justified.  Instead explicitly initialize the array when it is
> -   used.  */
> -#if 0
> -static const char *const okshells[] = { _PATH_BSHELL, _PATH_CSHELL, NULL };
> -#else
> -static const char *okshells[3];
> +/* Used by tests to point to custom paths.  */
> +#ifndef _LOCAL_PATH_SHELLS
> +# define _LOCAL_PATH_SHELLS _PATH_SHELLS
>  #endif
> -static char **curshell, **shells, *strings;
> -static char **initshells (void) __THROW;
>  
> -/*
> - * Get a list of shells from _PATH_SHELLS, if it exists.
> - */
> +/* These functions are not thread safe (by design), so globals are OK.  */
> +static FILE *shellfp;
> +static int okshell_idx;
> +static char *shellbuf;
> +static size_t buf_len;
> +
>  char *
>  getusershell (void)
>  {
> -	char *ret;
> -
> -	if (curshell == NULL)
> -		curshell = initshells();
> -	ret = *curshell;
> -	if (ret != NULL)
> -		curshell++;
> -	return (ret);
> +  /* Handle the builtin case first.
> +
> +     NB: we do not use a global array here as the initialization needs
> +     relocations.  These interfaces are used so rarely that this is not
> +     justified.  Instead open code it with a switch statement.  */
> +  switch (okshell_idx)
> +    {
> +    case 0:
> +      if (shellfp == NULL)
> +	{
> +	  setusershell ();
> +	  return getusershell ();
> +	}
> +      break;
> +    case 1:
> +      okshell_idx++;
> +      return (char *) _PATH_BSHELL;
> +    case 2:
> +      okshell_idx++;
> +      return (char *) _PATH_CSHELL;
> +    case 3:
> +      return NULL;
> +    }
> +
> +  /* Walk the /etc/shells file.  */
> +  while (1)
> +    {
> +      ssize_t len;
> +      char *p;
> +
> +      len = getline (&shellbuf, &buf_len, shellfp);
> +      if (len <= 0)
> +	break;
> +
> +      /* Chop the trailing newline.  */
> +      p = shellbuf + len - 1;
> +      if (*p == '\n')
> +	*p = '\0';
> +
> +      /* Only accept valid shells (which we define as starting with a '/').  */
> +      if (shellbuf[0] == '/')
> +	return shellbuf;
> +    }
> +
> +  /* No more valid shells.  */
> +  return NULL;
>  }
>  
> +hidden_proto (endusershell);
>  void
>  endusershell (void)
>  {
> -
> -	free(shells);
> -	shells = NULL;
> -	free(strings);
> -	strings = NULL;
> -	curshell = NULL;
> +  okshell_idx = 0;
> +  if (shellfp)
> +    {
> +      fclose (shellfp);
> +      shellfp = NULL;
> +      free (shellbuf);
> +      shellbuf = NULL;
> +    }
>  }
> +hidden_def (endusershell);
>  
>  void
>  setusershell (void)
>  {
> +  /* We could rewind shellfp here, but we get smaller code this way.
> +     Keep in mind this is not a performance sensitive API.  */
> +  endusershell ();
>  
> -	curshell = initshells();
> -}
> -
> -static char **
> -initshells (void)
> -{
> -	char **sp, *cp;
> -	FILE *fp;
> -	struct stat64 statb;
> -	size_t flen;
> -
> -	free(shells);
> -	shells = NULL;
> -	free(strings);
> -	strings = NULL;
> -	if ((fp = fopen(_PATH_SHELLS, "rce")) == NULL)
> -		goto init_okshells_noclose;
> -	if (fstat64(fileno(fp), &statb) == -1) {
> -	init_okshells:
> -		(void)fclose(fp);
> -	init_okshells_noclose:
> -		okshells[0] = _PATH_BSHELL;
> -		okshells[1] = _PATH_CSHELL;
> -		return (char **) okshells;
> -	}
> -	if (statb.st_size > ~(size_t)0 / sizeof (char *) * 3)
> -		goto init_okshells;
> -	flen = statb.st_size + 3;
> -	if ((strings = malloc(flen)) == NULL)
> -		goto init_okshells;
> -	shells = malloc(statb.st_size / 3 * sizeof (char *));
> -	if (shells == NULL) {
> -		free(strings);
> -		strings = NULL;
> -		goto init_okshells;
> -	}
> -	sp = shells;
> -	cp = strings;
> -	while (fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) {
> -		while (*cp != '#' && *cp != '/' && *cp != '\0')
> -			cp++;
> -		if (*cp == '#' || *cp == '\0' || cp[1] == '\0')
> -			continue;
> -		*sp++ = cp;
> -		while (!isspace(*cp) && *cp != '#' && *cp != '\0')
> -			cp++;
> -		*cp++ = '\0';
> -	}
> -	*sp = NULL;
> -	(void)fclose(fp);
> -	return (shells);
> +  shellfp = fopen (_LOCAL_PATH_SHELLS, "rce");
> +  if (shellfp == NULL)
> +    okshell_idx = 1;
>  }
> diff --git a/misc/tst-endusershell.c b/misc/tst-endusershell.c
> new file mode 100644
> index 0000000..95c2143
> --- /dev/null
> +++ b/misc/tst-endusershell.c
> @@ -0,0 +1,64 @@
> +/* Make sure calling endusershell first doesn't crash.
> +
> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
> +static int
> +do_test (void)
> +{
> +  /* This should not crash or corrupt.  */
> +  endusershell ();
> +  endusershell ();
> +  endusershell ();
> +
> +  /* Neither should this.  */
> +  setusershell ();
> +  setusershell ();
> +  setusershell ();
> +  setusershell ();
> +  setusershell ();
> +
> +  /* Shouldn't be a problem, but we can't really verify the return since
> +     it depends on /etc/shells in the installed system.  */
> +  getusershell ();
> +  getusershell ();
> +  getusershell ();
> +  getusershell ();
> +  getusershell ();
> +  getusershell ();
> +
> +  /* Test resetting behavior.  */
> +  setusershell ();
> +  getusershell ();
> +  setusershell ();
> +  getusershell ();
> +
> +  /* Test circular behavior.  */
> +  setusershell ();
> +  getusershell ();
> +  endusershell ();
> +  setusershell ();
> +  getusershell ();
> +  endusershell ();
> +
> +  return 0;
> +}
> diff --git a/misc/tst-getusershell.c b/misc/tst-getusershell.c
> new file mode 100644
> index 0000000..27fc4f8
> --- /dev/null
> +++ b/misc/tst-getusershell.c
> @@ -0,0 +1,178 @@
> +/* Verify behavior of getusershell w/various inputs.
> +
> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Set the functions to read from our custom file so we can control the
> +   exact content for the tests.  */
> +static char *path_shells;
> +#define _LOCAL_PATH_SHELLS path_shells
> +#include "getusershell.c"
> +
> +#include <assert.h>
> +
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
> +static int path_shells_fd;
> +
> +static void
> +set_content (const char *content)
> +{
> +  size_t len = strlen (content);
> +
> +  assert (lseek (path_shells_fd, 0, SEEK_SET) == 0);
> +  assert (ftruncate (path_shells_fd, 0) == 0);
> +  assert (write (path_shells_fd, content, len) == len);
> +
> +  /* Reset the shell reading state since we rewrote the file.  */
> +  endusershell ();
> +}
> +
> +#define dprintf(fmt, args...) printf ("%s: " fmt, __func__, ## args)
> +
> +/* Verify behavior of getusershell w/out any /etc/shells file.  */
> +static int
> +check_default (void)
> +{
> +  char bad_shells[] = "";
> +  char *old_shells = path_shells;
> +  const char *shell;
> +  int ret = 1, i;
> +
> +  /* This will cause fopen to fail.  */
> +  path_shells = bad_shells;
> +
> +  shell = getusershell ();
> +  if (shell == NULL || strcmp (shell, "/bin/sh") != 0)
> +    {
> +      dprintf ("first shell must be /bin/sh, not '%s'\n", shell);
> +      goto done;
> +    }
> +
> +  shell = getusershell ();
> +  if (shell == NULL || strcmp (shell, "/bin/csh") != 0)
> +    {
> +      dprintf ("second shell must be /bin/csh, not '%s'\n", shell);
> +      goto done;
> +    }
> +
> +  /* Call it a few times to make sure it doesn't wrap or anything.  */
> +  for (i = 0; i < 10; ++i)
> +    {
> +      shell = getusershell ();
> +      if (shell != NULL)
> +	{
> +	  dprintf ("third shell must be NULL, not '%s'\n", shell);
> +	  goto done;
> +	}
> +    }
> +
> +  ret = 0;
> + done:
> +  path_shells = old_shells;
> +  return ret;
> +}
> +
> +/* An empty file should return NULL forever.  */
> +static int
> +check_empty (void)
> +{
> +  int i;
> +  char *shell;
> +
> +  set_content ("");
> +
> +  for (i = 0; i < 10; ++i)
> +    {
> +      shell = getusershell ();
> +      if (shell != NULL)
> +	{
> +	  dprintf ("empty file should return NULL, not '%s'\n", shell);
> +	  return 1;
> +	}
> +    }
> +
> +  return 0;
> +}
> +
> +/* Verify we handle comments and blank lines correctly.  */
> +static int
> +check_comments_blank_lines (void)
> +{
> +  int i;
> +  char *shell;
> +
> +  set_content (
> +    /* A mix of comments and blanks.  */
> +    "# A comment\n"
> +    "  # A comment w/leading spaces\n"
> +    "\t\t# A comment w/leading tabs\n"
> +    /* Now just blank lines.  */
> +    "\n"
> +    " \n"
> +    "\t \t\n"
> +  );
> +
> +  for (i = 0; i < 10; ++i)
> +    {
> +      shell = getusershell ();
> +      if (shell != NULL)
> +	{
> +	  dprintf ("comment/blank file should return NULL, not '%s'\n", shell);
> +	  return 1;
> +	}
> +    }
> +
> +  return 0;
> +}
> +
> +/* Verify "valid" shells which is defined by starting with a /.  */
> +static int
> +check_valid_shells (void)
> +{
> +  char *shell;
> +
> +  set_content (
> +    "/bin/foo/bar/ll/pants\n"
> +  );
> +
> +  shell = getusershell ();
> +  if (shell == NULL || strcmp (shell, "/bin/foo/bar/ll/pants") != 0)
> +    {
> +      dprintf ("shell should be /bin/foo/bar/ll/pants, not '%s'\n", shell);
> +      return 1;
> +    }
> +
> +  return 0;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  path_shells_fd = create_temp_file ("getusershell", &path_shells);
> +  if (path_shells_fd < 0)
> +    return 1;
> +
> +  return
> +    check_default () +
> +    check_empty () +
> +    check_comments_blank_lines () +
> +    check_valid_shells ();
> +}
>

Patch

diff --git a/NEWS b/NEWS
index 1313ab2..4ba05e2 100644
--- a/NEWS
+++ b/NEWS
@@ -22,6 +22,10 @@  Version 2.23
   19049, 19050, 19059, 19071, 19074, 19076, 19077, 19078, 19079, 19085,
   19086, 19088, 19094, 19095, 19124, 19125, 19129, 19134, 19137.
 
+* The getusershell function now returns entries from /etc/shells only when
+  the line starts with a '/', and no longer chops lines based on '#' or white
+  space.
+
 * There is now a --disable-timezone-tools configure option for disabling the
   building and installing of the timezone related utilities (zic, zdump, and
   tzselect).  This is useful for people who build the timezone data and code
diff --git a/misc/Makefile b/misc/Makefile
index 2f5edf6..3fc44f1 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -77,7 +77,8 @@  gpl2lgpl := error.c error.h
 
 tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
 	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
-	 tst-mntent-blank-corrupt tst-mntent-blank-passno
+	 tst-mntent-blank-corrupt tst-mntent-blank-passno tst-getusershell \
+	 tst-endusershell
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out
 endif
diff --git a/misc/getusershell.c b/misc/getusershell.c
index fc2c43b..3d3f221 100644
--- a/misc/getusershell.c
+++ b/misc/getusershell.c
@@ -1,143 +1,112 @@ 
-/*
- * Copyright (c) 1985, 1993
- *	The Regents of the University of California.  All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- * 4. Neither the name of the University nor the names of its contributors
- *    may be used to endorse or promote products derived from this software
- *    without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
- * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
- * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
- * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
- * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-
-#if defined(LIBC_SCCS) && !defined(lint)
-static char sccsid[] = "@(#)getusershell.c	8.1 (Berkeley) 6/4/93";
-#endif /* LIBC_SCCS and not lint */
-
-#include <sys/param.h>
-#include <sys/file.h>
-#include <sys/stat.h>
+/* Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <assert.h>
+#include <paths.h>
 #include <stdio.h>
-#include <stdio_ext.h>
-#include <ctype.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
-#include <paths.h>
 
-/*
- * Local shells should NOT be added here.  They should be added in
- * /etc/shells.
- */
-
-/* NB: we do not initialize okshells here.  The initialization needs
-   relocations.  These interfaces are used so rarely that this is not
-   justified.  Instead explicitly initialize the array when it is
-   used.  */
-#if 0
-static const char *const okshells[] = { _PATH_BSHELL, _PATH_CSHELL, NULL };
-#else
-static const char *okshells[3];
+/* Used by tests to point to custom paths.  */
+#ifndef _LOCAL_PATH_SHELLS
+# define _LOCAL_PATH_SHELLS _PATH_SHELLS
 #endif
-static char **curshell, **shells, *strings;
-static char **initshells (void) __THROW;
 
-/*
- * Get a list of shells from _PATH_SHELLS, if it exists.
- */
+/* These functions are not thread safe (by design), so globals are OK.  */
+static FILE *shellfp;
+static int okshell_idx;
+static char *shellbuf;
+static size_t buf_len;
+
 char *
 getusershell (void)
 {
-	char *ret;
-
-	if (curshell == NULL)
-		curshell = initshells();
-	ret = *curshell;
-	if (ret != NULL)
-		curshell++;
-	return (ret);
+  /* Handle the builtin case first.
+
+     NB: we do not use a global array here as the initialization needs
+     relocations.  These interfaces are used so rarely that this is not
+     justified.  Instead open code it with a switch statement.  */
+  switch (okshell_idx)
+    {
+    case 0:
+      if (shellfp == NULL)
+	{
+	  setusershell ();
+	  return getusershell ();
+	}
+      break;
+    case 1:
+      okshell_idx++;
+      return (char *) _PATH_BSHELL;
+    case 2:
+      okshell_idx++;
+      return (char *) _PATH_CSHELL;
+    case 3:
+      return NULL;
+    }
+
+  /* Walk the /etc/shells file.  */
+  while (1)
+    {
+      ssize_t len;
+      char *p;
+
+      len = getline (&shellbuf, &buf_len, shellfp);
+      if (len <= 0)
+	break;
+
+      /* Chop the trailing newline.  */
+      p = shellbuf + len - 1;
+      if (*p == '\n')
+	*p = '\0';
+
+      /* Only accept valid shells (which we define as starting with a '/').  */
+      if (shellbuf[0] == '/')
+	return shellbuf;
+    }
+
+  /* No more valid shells.  */
+  return NULL;
 }
 
+hidden_proto (endusershell);
 void
 endusershell (void)
 {
-
-	free(shells);
-	shells = NULL;
-	free(strings);
-	strings = NULL;
-	curshell = NULL;
+  okshell_idx = 0;
+  if (shellfp)
+    {
+      fclose (shellfp);
+      shellfp = NULL;
+      free (shellbuf);
+      shellbuf = NULL;
+    }
 }
+hidden_def (endusershell);
 
 void
 setusershell (void)
 {
+  /* We could rewind shellfp here, but we get smaller code this way.
+     Keep in mind this is not a performance sensitive API.  */
+  endusershell ();
 
-	curshell = initshells();
-}
-
-static char **
-initshells (void)
-{
-	char **sp, *cp;
-	FILE *fp;
-	struct stat64 statb;
-	size_t flen;
-
-	free(shells);
-	shells = NULL;
-	free(strings);
-	strings = NULL;
-	if ((fp = fopen(_PATH_SHELLS, "rce")) == NULL)
-		goto init_okshells_noclose;
-	if (fstat64(fileno(fp), &statb) == -1) {
-	init_okshells:
-		(void)fclose(fp);
-	init_okshells_noclose:
-		okshells[0] = _PATH_BSHELL;
-		okshells[1] = _PATH_CSHELL;
-		return (char **) okshells;
-	}
-	if (statb.st_size > ~(size_t)0 / sizeof (char *) * 3)
-		goto init_okshells;
-	flen = statb.st_size + 3;
-	if ((strings = malloc(flen)) == NULL)
-		goto init_okshells;
-	shells = malloc(statb.st_size / 3 * sizeof (char *));
-	if (shells == NULL) {
-		free(strings);
-		strings = NULL;
-		goto init_okshells;
-	}
-	sp = shells;
-	cp = strings;
-	while (fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) {
-		while (*cp != '#' && *cp != '/' && *cp != '\0')
-			cp++;
-		if (*cp == '#' || *cp == '\0' || cp[1] == '\0')
-			continue;
-		*sp++ = cp;
-		while (!isspace(*cp) && *cp != '#' && *cp != '\0')
-			cp++;
-		*cp++ = '\0';
-	}
-	*sp = NULL;
-	(void)fclose(fp);
-	return (shells);
+  shellfp = fopen (_LOCAL_PATH_SHELLS, "rce");
+  if (shellfp == NULL)
+    okshell_idx = 1;
 }
diff --git a/misc/tst-endusershell.c b/misc/tst-endusershell.c
new file mode 100644
index 0000000..95c2143
--- /dev/null
+++ b/misc/tst-endusershell.c
@@ -0,0 +1,64 @@ 
+/* Make sure calling endusershell first doesn't crash.
+
+   Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+static int do_test (void);
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
+static int
+do_test (void)
+{
+  /* This should not crash or corrupt.  */
+  endusershell ();
+  endusershell ();
+  endusershell ();
+
+  /* Neither should this.  */
+  setusershell ();
+  setusershell ();
+  setusershell ();
+  setusershell ();
+  setusershell ();
+
+  /* Shouldn't be a problem, but we can't really verify the return since
+     it depends on /etc/shells in the installed system.  */
+  getusershell ();
+  getusershell ();
+  getusershell ();
+  getusershell ();
+  getusershell ();
+  getusershell ();
+
+  /* Test resetting behavior.  */
+  setusershell ();
+  getusershell ();
+  setusershell ();
+  getusershell ();
+
+  /* Test circular behavior.  */
+  setusershell ();
+  getusershell ();
+  endusershell ();
+  setusershell ();
+  getusershell ();
+  endusershell ();
+
+  return 0;
+}
diff --git a/misc/tst-getusershell.c b/misc/tst-getusershell.c
new file mode 100644
index 0000000..27fc4f8
--- /dev/null
+++ b/misc/tst-getusershell.c
@@ -0,0 +1,178 @@ 
+/* Verify behavior of getusershell w/various inputs.
+
+   Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Set the functions to read from our custom file so we can control the
+   exact content for the tests.  */
+static char *path_shells;
+#define _LOCAL_PATH_SHELLS path_shells
+#include "getusershell.c"
+
+#include <assert.h>
+
+static int do_test (void);
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
+static int path_shells_fd;
+
+static void
+set_content (const char *content)
+{
+  size_t len = strlen (content);
+
+  assert (lseek (path_shells_fd, 0, SEEK_SET) == 0);
+  assert (ftruncate (path_shells_fd, 0) == 0);
+  assert (write (path_shells_fd, content, len) == len);
+
+  /* Reset the shell reading state since we rewrote the file.  */
+  endusershell ();
+}
+
+#define dprintf(fmt, args...) printf ("%s: " fmt, __func__, ## args)
+
+/* Verify behavior of getusershell w/out any /etc/shells file.  */
+static int
+check_default (void)
+{
+  char bad_shells[] = "";
+  char *old_shells = path_shells;
+  const char *shell;
+  int ret = 1, i;
+
+  /* This will cause fopen to fail.  */
+  path_shells = bad_shells;
+
+  shell = getusershell ();
+  if (shell == NULL || strcmp (shell, "/bin/sh") != 0)
+    {
+      dprintf ("first shell must be /bin/sh, not '%s'\n", shell);
+      goto done;
+    }
+
+  shell = getusershell ();
+  if (shell == NULL || strcmp (shell, "/bin/csh") != 0)
+    {
+      dprintf ("second shell must be /bin/csh, not '%s'\n", shell);
+      goto done;
+    }
+
+  /* Call it a few times to make sure it doesn't wrap or anything.  */
+  for (i = 0; i < 10; ++i)
+    {
+      shell = getusershell ();
+      if (shell != NULL)
+	{
+	  dprintf ("third shell must be NULL, not '%s'\n", shell);
+	  goto done;
+	}
+    }
+
+  ret = 0;
+ done:
+  path_shells = old_shells;
+  return ret;
+}
+
+/* An empty file should return NULL forever.  */
+static int
+check_empty (void)
+{
+  int i;
+  char *shell;
+
+  set_content ("");
+
+  for (i = 0; i < 10; ++i)
+    {
+      shell = getusershell ();
+      if (shell != NULL)
+	{
+	  dprintf ("empty file should return NULL, not '%s'\n", shell);
+	  return 1;
+	}
+    }
+
+  return 0;
+}
+
+/* Verify we handle comments and blank lines correctly.  */
+static int
+check_comments_blank_lines (void)
+{
+  int i;
+  char *shell;
+
+  set_content (
+    /* A mix of comments and blanks.  */
+    "# A comment\n"
+    "  # A comment w/leading spaces\n"
+    "\t\t# A comment w/leading tabs\n"
+    /* Now just blank lines.  */
+    "\n"
+    " \n"
+    "\t \t\n"
+  );
+
+  for (i = 0; i < 10; ++i)
+    {
+      shell = getusershell ();
+      if (shell != NULL)
+	{
+	  dprintf ("comment/blank file should return NULL, not '%s'\n", shell);
+	  return 1;
+	}
+    }
+
+  return 0;
+}
+
+/* Verify "valid" shells which is defined by starting with a /.  */
+static int
+check_valid_shells (void)
+{
+  char *shell;
+
+  set_content (
+    "/bin/foo/bar/ll/pants\n"
+  );
+
+  shell = getusershell ();
+  if (shell == NULL || strcmp (shell, "/bin/foo/bar/ll/pants") != 0)
+    {
+      dprintf ("shell should be /bin/foo/bar/ll/pants, not '%s'\n", shell);
+      return 1;
+    }
+
+  return 0;
+}
+
+static int
+do_test (void)
+{
+  path_shells_fd = create_temp_file ("getusershell", &path_shells);
+  if (path_shells_fd < 0)
+    return 1;
+
+  return
+    check_default () +
+    check_empty () +
+    check_comments_blank_lines () +
+    check_valid_shells ();
+}