Re: [PATCH] getusershell: rewrite from scratch [BZ #18660]

[Tolga Dalman <tolga dot dalman at googlemail dot com>]

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 ();
> +}
>