On Sat, Dec 05, 2015 at 01:25:10PM -0500, Ted Unangst wrote:
> Tobias Stoeckmann wrote:
> >
> > And I still think that the current code is a bit too permissive in parsing
> > things. I mean what's the point in allowing lines like:
> >
> > sometextwithoutspace/bin/ksh should be used for logins # seriously!
> >
> > Which would result in /bin/ksh, by the way.
> >
> > Didn't notice the consequences that arise by keeping the descriptor open,
> > so I'm fine with an alternative approach. Yet we might make the code a
> > bit easier to review by not allowing such weird lines. What it should
> > expect and enforce:
> >
> > - a valid line has to start with a slash
> > - comments are chopped off
> > - comments are supposed to be at the beginning of a line
> >
> > So if somebody writes "/bin/ksh # comment", that actually leads to
> > "/bin/ksh ",
> > with an additional whitespace at the end. Currently we couldn't even
> > specify a
> > shell with a whitespace in its path.
>
> ok. i was going to leave the behavior alone, but we can fix that too.
>
> - use getline to read lines of any length.
> - only consider lines that start with a /.
> - truncate lines after a #, but not after spaces.
>
>
> Index: gen/getusershell.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/getusershell.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 getusershell.c
> --- gen/getusershell.c 14 Sep 2015 16:09:13 -0000 1.16
> +++ gen/getusershell.c 5 Dec 2015 18:24:33 -0000
> @@ -28,14 +28,13 @@
> * SUCH DAMAGE.
> */
>
> -#include <sys/stat.h>
> -
> #include <ctype.h>
> #include <limits.h>
> #include <paths.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> +#include <string.h>
> #include <unistd.h>
>
> /*
> @@ -44,7 +43,7 @@
> */
>
> static char *okshells[] = { _PATH_BSHELL, _PATH_CSHELL, _PATH_KSHELL, NULL };
> -static char **curshell, **shells, *strings;
> +static char **curshell, **shells;
> static char **initshells(void);
>
> /*
> @@ -66,11 +65,14 @@ getusershell(void)
> void
> endusershell(void)
> {
> -
> + char **s;
> +
> + if ((s = shells))
> + while (*s)
> + free(*s++);
> free(shells);
> shells = NULL;
> - free(strings);
> - strings = NULL;
> +
> curshell = NULL;
> }
>
> @@ -84,48 +86,50 @@ setusershell(void)
> static char **
> initshells(void)
> {
> - char **sp, *cp;
> + size_t nshells, nalloc, linesize;
> + char *line;
> FILE *fp;
> - struct stat statb;
>
> free(shells);
> shells = NULL;
> - free(strings);
> - strings = NULL;
> +
> if ((fp = fopen(_PATH_SHELLS, "re")) == NULL)
> return (okshells);
> - if (fstat(fileno(fp), &statb) == -1) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - if (statb.st_size > SIZE_MAX) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - if ((strings = malloc((size_t)statb.st_size)) == NULL) {
> - (void)fclose(fp);
> - return (okshells);
> - }
> - shells = calloc((size_t)(statb.st_size / 3 + 2), sizeof (char *));
> - if (shells == NULL) {
> - (void)fclose(fp);
> - free(strings);
> - strings = NULL;
> - return (okshells);
> - }
> - sp = shells;
> - cp = strings;
> - while (fgets(cp, PATH_MAX + 1, fp) != NULL) {
> - while (*cp != '#' && *cp != '/' && *cp != '\0')
> - cp++;
> - if (*cp == '#' || *cp == '\0')
> +
> + line = NULL;
> + nalloc = 10; // just an initial guess
> + nshells = 0;
> + shells = reallocarray(NULL, nalloc, sizeof (char *));
> + if (shells == NULL)
> + goto fail;
> + linesize = 0;
> + while (getline(&line, &linesize, fp) != -1) {
> + if (*line != '/')
> continue;
> - *sp++ = cp;
> - while (!isspace((unsigned char)*cp) && *cp != '#' && *cp !=
> '\0')
> - cp++;
> - *cp++ = '\0';
> + line[strcspn(line, "#\n")] = '\0';
> + if (!(shells[nshells] = strdup(line)))
> + goto fail;
> +
> + nshells++;
> + if (nshells == nalloc) {
> + char **new = reallocarray(shells, nalloc * 2,
> sizeof(char *));
> + if (!new)
> + goto fail;
This 'goto fail' will free() beyond allocated: shells[nshells--]
Better to check 'if (nshells + 1 == nalloc)' and increment nshells
afterward.
--patrick
> + shells = new;
> + nalloc *= 2;
> + }
> }
> - *sp = NULL;
> + free(line);
> + shells[nshells] = NULL;
> (void)fclose(fp);
> return (shells);
> +
> +fail:
> + free(line);
> + while (nshells)
> + free(shells[nshells--]);
> + free(shells);
> + shells = NULL;
> + (void)fclose(fp);
> + return (okshells);
> }
>
