From patchwork Wed Oct 21 05:39:04 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Frysinger X-Patchwork-Id: 9284 X-Patchwork-Delegate: vapier@gentoo.org Received: (qmail 7931 invoked by alias); 21 Oct 2015 05:39:21 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 7910 invoked by uid 89); 21 Oct 2015 05:39:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL, BAYES_05, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: smtp.gentoo.org From: Mike Frysinger To: libc-alpha@sourceware.org Subject: [PATCH] getusershell: rewrite from scratch [BZ #18660] Date: Wed, 21 Oct 2015 01:39:04 -0400 Message-Id: <1445405944-19916-1-git-send-email-vapier@gentoo.org> In-Reply-To: References: 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 [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 -#include -#include +/* 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 + . */ + +#include +#include #include -#include -#include #include +#include #include -#include -/* - * 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 + . */ + +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 + . */ + +/* 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 + +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 (); +}