From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14449 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v2] glob: implement GLOB_TILDE Date: Wed, 24 Jul 2019 23:48:14 -0400 Message-ID: <20190725034814.GI1506@brightrain.aerifal.cx> References: <20190724213338.27138-1-ismael@iodev.co.uk> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="55701"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-14465-gllmg-musl=m.gmane.org@lists.openwall.com Thu Jul 25 05:48:32 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hqUk4-000ENx-C5 for gllmg-musl@m.gmane.org; Thu, 25 Jul 2019 05:48:32 +0200 Original-Received: (qmail 22526 invoked by uid 550); 25 Jul 2019 03:48:28 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 22507 invoked from network); 25 Jul 2019 03:48:28 -0000 Content-Disposition: inline In-Reply-To: <20190724213338.27138-1-ismael@iodev.co.uk> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:14449 Archived-At: On Wed, Jul 24, 2019 at 11:33:38PM +0200, Ismael Luceno wrote: > Signed-off-by: Ismael Luceno > --- > 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 > #include > #include > -#include > #include > #include > #include > +#include > +#include > +#define _GNU_SOURCE > +#include 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