From: Rich Felker <dalias@libc.org>
To: musl@lists.openwall.com
Subject: Re: [PATCH v2] glob: implement GLOB_TILDE
Date: Wed, 24 Jul 2019 23:48:14 -0400 [thread overview]
Message-ID: <20190725034814.GI1506@brightrain.aerifal.cx> (raw)
In-Reply-To: <20190724213338.27138-1-ismael@iodev.co.uk>
On Wed, Jul 24, 2019 at 11:33:38PM +0200, Ismael Luceno wrote:
> Signed-off-by: Ismael Luceno <ismael@iodev.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
next prev parent reply other threads:[~2019-07-25 3:48 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-24 21:33 Ismael Luceno
2019-07-25 3:48 ` Rich Felker [this message]
2019-07-25 10:33 ` Ismael Luceno
2019-07-25 14:30 ` Rich Felker
2019-07-25 15:39 ` Ismael Luceno
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190725034814.GI1506@brightrain.aerifal.cx \
--to=dalias@libc.org \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).