From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14437 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] glob: implement GLOB_TILDE Date: Tue, 23 Jul 2019 16:07:08 -0400 Message-ID: <20190723200708.GA25787@brightrain.aerifal.cx> References: <20190723183354.31633-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="163480"; 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-14453-gllmg-musl=m.gmane.org@lists.openwall.com Tue Jul 23 22:07:24 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 1hq14G-000gOu-DH for gllmg-musl@m.gmane.org; Tue, 23 Jul 2019 22:07:24 +0200 Original-Received: (qmail 22059 invoked by uid 550); 23 Jul 2019 20:07:21 -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 22034 invoked from network); 23 Jul 2019 20:07:20 -0000 Content-Disposition: inline In-Reply-To: <20190723183354.31633-1-ismael@iodev.co.uk> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:14437 Archived-At: On Tue, Jul 23, 2019 at 08:33:54PM +0200, Ismael Luceno wrote: > Signed-off-by: Ismael Luceno > --- > include/glob.h | 1 + > src/regex/glob.c | 26 ++++++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > 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..77b81f707420 100644 > --- a/src/regex/glob.c > +++ b/src/regex/glob.c > @@ -8,6 +8,8 @@ > #include > #include > #include > +#include > +#include "../passwd/pwf.h" > > struct match > { > @@ -35,6 +37,30 @@ static int do_glob(char *buf, size_t pos, int type, char *pat, int flags, int (* > /* If GLOB_MARK is unused, we don't care about type. */ > if (!type && !(flags & GLOB_MARK)) type = DT_REG; > > + if ((flags & GLOB_TILDE) && *pat == '~') { > + struct passwd pw, *res; > + char *buf = NULL; > + size_t len = 0; > + char *name_end = strchr(++pat, '/'); > + if (name_end) > + *name_end = 0; > + uid_t uid = *pat ? 0 : getuid(); > + int err = __getpw_a(pat, uid, &pw, &buf, &len, &res); > + if (!err && res) { > + while (pos < PATH_MAX - 1 && *pw.pw_dir) > + buf[pos++] = *pw.pw_dir++; > + } > + free(buf); > + if (err) > + return GLOB_NOSPACE; > + if (!res) > + return 0; > + if (name_end) { > + pat = name_end; > + *pat = '/'; > + } > + } This should almost surely go in the top-level glob function rather than the recusive function, where the original strdup of pat happens. As you've written it, I think it wrongly applies tilde expansion to each path component into which recursion happens, but not to ones where it doesn't, whereas it should only apply to the first component. Doing this in the recursive function also has the problem of exploding stack usage. One thing I don't see is how the code is working at all right now, because you've shadowed the argument buf with a locally scoped variable by the same name that's the working space buffer passed to __getpw_a. The result directory is never copied out into the actual buf array. One thing to note at this point: it's generally preferable to use the public interfaces (getpwnam_r) rather than the internal ones from a different sybsystem of libc (__getpw_a) unless there's a really compelling reason not to. Here I think it would actually be easier using the public interface -- you could reuse buf for the buffer space you pass to getpwnam_r, then memmove from pw_dir (which lies somewhere inside buf, you don't know where) to the start of buf. There's also a matter of intended behavor: my understanding is that plain ~ without a name is supposed to expand to $HOME, not lookup the caller's passwd entry based on uid (which is broken on setups where you have multiple login accounts with the same uid). The man page doesn't document this at all. I looked just now in the glibc manual and it documents the behavior in terms of getlogin+getpwnam, which is utterly broken -- it doesn't work at all in the absence of a login session associated with a terminal (except in musl where we wrongly just return getenv("LOGNAME"), a known/open bug). and it's also inconsistent with the standard shell behavior, which is to use $HOME: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_01 So I feel like, if we implement GLOB_TILDE, we should do it the way that's consistent with the standard meaning of ~, using $HOME, not the glibc implementation. This is also much more user-friendly, in that it honors manual override of $HOME. Rich