Message ID | CAPC3xaqRbCc1Ytd7AAkmEjgr_wtq_AA-hEvRPZiup2QwHMdS-w@mail.gmail.com |
---|---|
State | New |
Headers | show |
On 18 Aug 2015 16:54, Paul Pluzhnikov wrote: > while (fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) { > while (*cp != '#' && *cp != '/' && *cp != '\0') > cp++; > - if (*cp == '#' || *cp == '\0' || cp[1] == '\0') > + /* Reject non-absolute paths, or anything too short. */ > + if (cp[0] != '/' || cp[1] == '\0' || isspace(cp[1])) > continue; > *sp++ = cp; > while (!isspace(*cp) && *cp != '#' && *cp != '\0') > cp++; > + assert (cp < strings + flen); > *cp++ = '\0'; > } i'm having a hard time seeing how this works sanely (even before your change). considering this file starts off with this comment: /* 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. */ how can we justify this ugly code in the name of speed ? wouldn't it be nicer to gut it completely and avoid the stat/large malloc/etc... ? lightly tested from scratch version below. code & data size is smaller than the current version in the tree. -mike /* 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 <stdlib.h> #include <string.h> #include <unistd.h> /* Used by tests to point to custom paths. */ #ifndef _LOCAL_PATH_SHELL # define _LOCAL_PATH_SHELLS _PATH_SHELLS #endif /* 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) { /* 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: /* Require the user call setusershell first. */ assert (shellfp != NULL); 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 (getline (&shellbuf, &buf_len, shellfp) != -1) { char *p; /* Strip out any comment lines. */ p = strchr (shellbuf, '#'); if (p) *p = '\0'; else { /* Chop the trailing newline. */ p = strchr (shellbuf, '\n'); if (p) *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) { 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 (); shellfp = fopen (_LOCAL_PATH_SHELLS, "rce"); if (shellfp == NULL) okshell_idx = 1; }
Mike Frysinger <vapier@gentoo.org> writes:
> how can we justify this ugly code in the name of speed ?
This ugly code was lifted directly from BSD ...
Andreas.
On 19 Aug 2015 17:26, Andreas Schwab wrote: > Mike Frysinger <vapier@gentoo.org> writes: > > how can we justify this ugly code in the name of speed ? > > This ugly code was lifted directly from BSD ... i'm aware of its history. that doesn't justify us keeping the ugly code, just the API. -mike
On Wed, Aug 19, 2015 at 12:19 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On 19 Aug 2015 17:26, Andreas Schwab wrote: >> Mike Frysinger <vapier@gentoo.org> writes: >> > how can we justify this ugly code in the name of speed ? >> >> This ugly code was lifted directly from BSD ... > > i'm aware of its history. that doesn't justify us keeping the ugly code, > just the API. FWIW I concur with Mike. zw
On Wed, Aug 19, 2015 at 7:55 AM, Mike Frysinger <vapier@gentoo.org> wrote: > lightly tested from scratch version below. code & data size is smaller than > the current version in the tree. > -mike > > /* 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 <stdlib.h> > #include <string.h> > #include <unistd.h> > > /* Used by tests to point to custom paths. */ > #ifndef _LOCAL_PATH_SHELL > # define _LOCAL_PATH_SHELLS _PATH_SHELLS > #endif > > /* 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) > { > /* 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: > /* Require the user call setusershell first. */ > assert (shellfp != NULL); I could find no man page that requires user calling setusershell first. Is there a definitive description of this interface? > 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 (getline (&shellbuf, &buf_len, shellfp) != -1) > { > char *p; > > /* Strip out any comment lines. */ > p = strchr (shellbuf, '#'); > if (p) > *p = '\0'; > else > { > /* Chop the trailing newline. */ > p = strchr (shellbuf, '\n'); Current version chops on white space. Do we want to return "/foo bar" here? Of course any sysadmin who puts "/foo bar" into /etc/shells gets what he deserves. > if (p) > *p = '\0'; > } > > /* Only accept valid shells (which we define as starting with a '/'). */ > if (shellbuf[0] == '/') Do we want to return "/" as a valid shell here? > return shellbuf; > } > > /* No more valid shells. */ > return NULL; > } > > hidden_proto (endusershell); > void > endusershell (void) > { > 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 (); > > shellfp = fopen (_LOCAL_PATH_SHELLS, "rce"); > if (shellfp == NULL) > okshell_idx = 1; I think you missed a part here: else okshell_idx = 4; > }
On 19 Aug 2015 09:28, Paul Pluzhnikov wrote: > On Wed, Aug 19, 2015 at 7:55 AM, Mike Frysinger wrote: > > /* 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) > > { > > /* 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: > > /* Require the user call setusershell first. */ > > assert (shellfp != NULL); > > I could find no man page that requires user calling setusershell first. > Is there a definitive description of this interface? hmm, looks like freebsd, openbsd, darwin, and gnulib don't require this. i'll drop that. > > while (getline (&shellbuf, &buf_len, shellfp) != -1) > > { > > char *p; > > > > /* Strip out any comment lines. */ > > p = strchr (shellbuf, '#'); > > if (p) > > *p = '\0'; > > else > > { > > /* Chop the trailing newline. */ > > p = strchr (shellbuf, '\n'); > > Current version chops on white space. Do we want to return "/foo bar" > here? Of course any sysadmin who puts "/foo bar" into /etc/shells gets > what he deserves. the old logic is even worse than that. it skips all leading bytes until it finds a slash and then returns things after that, while splitting on space. so a line like: space/foobar cow will return "/foobar". the docs i've seen (darwin, freebsd, linux) leave it fairly vague and say "valid user shell". but no one validates things, and really only document comments and blank lines. i think we should just go with that. > > /* Only accept valid shells (which we define as starting with a '/'). */ > > if (shellbuf[0] == '/') > > Do we want to return "/" as a valid shell here? the current already does :). since we aren't checking the path in the file (i.e. running stat and seeing if it's a +x file), i don't think we want to treat / specially. afterall, "/bogus" or "/bin" or "/sbin/" will still get returned. > > 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 (); > > > > shellfp = fopen (_LOCAL_PATH_SHELLS, "rce"); > > if (shellfp == NULL) > > okshell_idx = 1; > > I think you missed a part here: > > else > okshell_idx = 4; nope ... current code is correct. see how the value of 0 is treated (which is also the default since it's a bss var). -mike
Mike, your proposal looks good to me (I read it and wrote a small test program). In fact, I see it as a great improvement to the status quo. Have you integrated your version and tried to add a test case as proposed earlier ? Best regards Tolga Dalman On 08/19/2015 08:27 PM, Mike Frysinger wrote: > On 19 Aug 2015 09:28, Paul Pluzhnikov wrote: >> On Wed, Aug 19, 2015 at 7:55 AM, Mike Frysinger wrote: >>> /* 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) >>> { >>> /* 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: >>> /* Require the user call setusershell first. */ >>> assert (shellfp != NULL); >> >> I could find no man page that requires user calling setusershell first. >> Is there a definitive description of this interface? > > hmm, looks like freebsd, openbsd, darwin, and gnulib don't require this. > i'll drop that. > >>> while (getline (&shellbuf, &buf_len, shellfp) != -1) >>> { >>> char *p; >>> >>> /* Strip out any comment lines. */ >>> p = strchr (shellbuf, '#'); >>> if (p) >>> *p = '\0'; >>> else >>> { >>> /* Chop the trailing newline. */ >>> p = strchr (shellbuf, '\n'); >> >> Current version chops on white space. Do we want to return "/foo bar" >> here? Of course any sysadmin who puts "/foo bar" into /etc/shells gets >> what he deserves. > > the old logic is even worse than that. it skips all leading bytes until it > finds a slash and then returns things after that, while splitting on space. > so a line like: > space/foobar cow > will return "/foobar". > > the docs i've seen (darwin, freebsd, linux) leave it fairly vague and say > "valid user shell". but no one validates things, and really only document > comments and blank lines. i think we should just go with that. > >>> /* Only accept valid shells (which we define as starting with a '/'). */ >>> if (shellbuf[0] == '/') >> >> Do we want to return "/" as a valid shell here? > > the current already does :). since we aren't checking the path in the file > (i.e. running stat and seeing if it's a +x file), i don't think we want to > treat / specially. afterall, "/bogus" or "/bin" or "/sbin/" will still get > returned. > >>> 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 (); >>> >>> shellfp = fopen (_LOCAL_PATH_SHELLS, "rce"); >>> if (shellfp == NULL) >>> okshell_idx = 1; >> >> I think you missed a part here: >> >> else >> okshell_idx = 4; > > nope ... current code is correct. see how the value of 0 is treated (which is > also the default since it's a bss var). > -mike >
On 26 Aug 2015 23:07, Tolga Dalman wrote: > your proposal looks good to me (I read it and wrote a small test program). > In fact, I see it as a great improvement to the status quo. > > Have you integrated your version and tried to add a test case as proposed earlier ? all i've done is post this patch. if people are happy with it and the direction, i can add some tests. -mike
> all i've done is post this patch. if people are happy with it and the > direction, i can add some tests. I think the comment handling could be further adjusted. After all, it IS possible to have a shell path containing '#' or ' '. I won't argue that it's a sane thing to do, but the file system allows it. And right now it's impossible to specify such a shell. It is also specified that a valid shell must start with '/', i.e. it must be an absolute path. So, how about ignoring every line that does not define an absolute path? That way, comments are supported, too. Unfortunately I don't know if anyone writes comments next to the absolute path in the file, like: /bin/sh # korn shell
diff --git a/misc/getusershell.c b/misc/getusershell.c index fc2c43b..a5d861e 100644 --- a/misc/getusershell.c +++ b/misc/getusershell.c @@ -31,6 +31,7 @@ static char sccsid[] = "@(#)getusershell.c 8.1 (Berkeley) 6/4/93"; #endif /* LIBC_SCCS and not lint */ +#include <assert.h> #include <sys/param.h> #include <sys/file.h> #include <sys/stat.h> @@ -99,6 +100,7 @@ initshells (void) FILE *fp; struct stat64 statb; size_t flen; + off64_t max_shells; free(shells); shells = NULL; @@ -114,12 +116,13 @@ initshells (void) okshells[1] = _PATH_CSHELL; return (char **) okshells; } - if (statb.st_size > ~(size_t)0 / sizeof (char *) * 3) + max_shells = (statb.st_size / 3) + 2; + if (max_shells > ~(size_t)0 / sizeof (char *)) goto init_okshells; flen = statb.st_size + 3; if ((strings = malloc(flen)) == NULL) goto init_okshells; - shells = malloc(statb.st_size / 3 * sizeof (char *)); + shells = malloc(max_shells * sizeof (char *)); if (shells == NULL) { free(strings); strings = NULL; @@ -130,13 +133,16 @@ initshells (void) while (fgets_unlocked(cp, flen - (cp - strings), fp) != NULL) { while (*cp != '#' && *cp != '/' && *cp != '\0') cp++; - if (*cp == '#' || *cp == '\0' || cp[1] == '\0') + /* Reject non-absolute paths, or anything too short. */ + if (cp[0] != '/' || cp[1] == '\0' || isspace(cp[1])) continue; *sp++ = cp; while (!isspace(*cp) && *cp != '#' && *cp != '\0') cp++; + assert (cp < strings + flen); *cp++ = '\0'; } + assert (sp < shells + max_shells); *sp = NULL; (void)fclose(fp); return (shells);