[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Jul 2019 23:48:14 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v2] glob: implement GLOB_TILDE
On Wed, Jul 24, 2019 at 11:33:38PM +0200, Ismael Luceno wrote:
> Signed-off-by: Ismael Luceno <ismael@...ev.co.uk>
> ---
> include/glob.h | 1 +
> src/regex/glob.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/include/glob.h b/include/glob.h
> index 76f6c1c68a23..fc8106b20c5b 100644
> --- a/include/glob.h
> +++ b/include/glob.h
> @@ -30,6 +30,7 @@ void globfree(glob_t *);
> #define GLOB_APPEND 0x20
> #define GLOB_NOESCAPE 0x40
> #define GLOB_PERIOD 0x80
> +#define GLOB_TILDE 0x100
>
> #define GLOB_NOSPACE 1
> #define GLOB_ABORTED 2
> diff --git a/src/regex/glob.c b/src/regex/glob.c
> index aa1c6a4482ee..2428d5f02c2e 100644
> --- a/src/regex/glob.c
> +++ b/src/regex/glob.c
> @@ -4,10 +4,13 @@
> #include <sys/stat.h>
> #include <dirent.h>
> #include <limits.h>
> -#include <string.h>
> #include <stdlib.h>
> #include <errno.h>
> #include <stddef.h>
> +#include <unistd.h>
> +#include <pwd.h>
> +#define _GNU_SOURCE
> +#include <string.h>
If feature test macros are used, they have to appear before any
headers are included (at the top of the file). But you can't call
GNU-namespace functions from a standard-namespace function (glob)
anyway. Instead just use __strchrnul if you need it; it's available
(musl-internal only) without any special FTMs.
>
> struct match
> {
> @@ -182,6 +185,39 @@ static int sort(const void *a, const void *b)
> return strcmp(*(const char **)a, *(const char **)b);
> }
>
> +static int expand_tilde(char **pat, char *buf, size_t *pos)
> +{
I like that you've put this in a separate function.
> + char *p = *pat + 1;
> + size_t i = 0;
> +
> + char *name_end = strchrnul(p, '/');
> + if (*name_end) *name_end++ = 0;
> + *pat = name_end;
> +
> + char *home = (*p || issetugid()) ? NULL : getenv("HOME");
I still don't see any justification for violating the semantics of ~
because the program is suid.
Also, issetugid isn't in the namespace we can use here. Rather than
poking directly at libc.secure, if we needed this I think it would be
better to make a namespace-protected alias, but I don't think it's the
right behavior anyway.
> + if (!home) {
> + struct passwd pw, *res;
> + switch (*p ? getpwnam_r(p, &pw, buf, PATH_MAX, &res)
> + : getpwuid_r(getuid(), &pw, buf, PATH_MAX, &res)) {
Same here. I'm not actually sure what should happen if $HOME is unset,
but it's probably whatever the Shell Command Language specification
says.
> + default:
> + return GLOB_ABORTED;
> + case ERANGE: case ENOMEM:
> + return GLOB_NOSPACE;
> + case 0:
> + if (!res) return GLOB_NOMATCH;
The man page says:
If the username is invalid, or the home directory cannot be
determined, then no substitution is performed.
and the glibc manual says:
If the user name is not valid or the home directory cannot be
determined for some reason the pattern is left untouched and
itself used as the result
So I think this should not be an error. I'm not entirely clear if it's
supposed to suppress all further glob expansion and just return a
single literal result, or treat "~" or "~nosuchuser" as a literal
component and search it.
> + }
> + home = pw.pw_dir;
> + }
> + while (i < PATH_MAX - 2 && *home)
> + buf[i++] = *home++;
This could be strnlen+memmove, but perhaps just open coding it like
you've done is simpler and lighter.
> + if (i > PATH_MAX - 2)
Off-by-one. If it stopped due to reaching the limit in the above loop,
you'll have equality not greater-than, then overflow below. Perhaps
strnlen+memmove would be easier to ensure the safety of.
> + return GLOB_NOSPACE;
GLOB_NOSPACE is for allocation errors. This is a no-match case, since
there can be no matches for a pattern PATH_MAX or longer.
> + buf[i++] = '/';
> + buf[i] = 0;
> + *pos = i;
> + return 0;
> +}
> +
> int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, int err), glob_t *restrict g)
> {
> struct match head = { .next = NULL }, *tail = &head;
> @@ -202,7 +238,12 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
> char *p = strdup(pat);
> if (!p) return GLOB_NOSPACE;
> buf[0] = 0;
> - error = do_glob(buf, 0, 0, p, flags, errfunc, &tail);
> + size_t pos = 0;
> + char *s = p;
> + if ((flags & GLOB_TILDE) && *p == '~')
> + error = expand_tilde(&s, buf, &pos);
> + if (!error)
> + error = do_glob(buf, pos, 0, s, flags, errfunc, &tail);
> free(p);
> }
This part looks good, but may need minor rework depending on how the
nonexistent user case is supposed to behave.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
