> 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 16:08:56 -0000
> @@ -84,48 +86,52 @@ setusershell(void)
> static char **
> initshells(void)
> {
> - char **sp, *cp;
> + char buf[PATH_MAX];
> + int nshells = 0, nalloc;
I would prefer size_t for nshells and nalloc.
> + char *cp;
> 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) {
> +
> + nalloc = 10; // just an initial guess
> + nshells = 0;
> + shells = reallocarray(NULL, nalloc, sizeof (char *));
> + if (shells == NULL)
> + goto fail;
> + while ((cp = fgets(buf, sizeof(buf), fp)) != NULL) {
We already have to dynamically allocate memory anyway, so getline() would
fix some issues we could face while parsing files. The buffer is PATH_MAX
bytes long, which should be sufficient, but if a comment is PATH_MAX + x
bytes in size, we would parse the "x" part as a real path due to fgets'
truncation/wrapping.
> while (*cp != '#' && *cp != '/' && *cp != '\0')
> cp++;
> if (*cp == '#' || *cp == '\0')
> continue;
> - *sp++ = cp;
> + if (!(shells[nshells] = strdup(cp)))
> + goto fail;
> + cp = shells[nshells];
> while (!isspace((unsigned char)*cp) && *cp != '#' && *cp !=
> '\0')
> cp++;
> *cp++ = '\0';
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.
Tobias
