From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14439 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Ismael Luceno Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] glob: implement GLOB_TILDE Date: Wed, 24 Jul 2019 09:15:22 +0200 Message-ID: <20190724071522.GA24370@pirotess.home> References: <20190723183354.31633-1-ismael@iodev.co.uk> <20190723200708.GA25787@brightrain.aerifal.cx> 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="137115"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.12.1 (2019-06-15) To: musl@lists.openwall.com Original-X-From: musl-return-14455-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jul 24 09:15:41 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 1hqBUw-000ZWq-5T for gllmg-musl@m.gmane.org; Wed, 24 Jul 2019 09:15:38 +0200 Original-Received: (qmail 16042 invoked by uid 550); 24 Jul 2019 07:15:36 -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 16024 invoked from network); 24 Jul 2019 07:15:35 -0000 Content-Disposition: inline In-Reply-To: <20190723200708.GA25787@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14439 Archived-At: On 23/Jul/2019 16:07, Rich Felker wrote: > 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. Indeed it doesn't work XD. I meant it more as a quick try and to see if it was interesting and the right kind of approach. Though I forgot to add "RFC" to the subject prefix. > 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. The reason I had to use the private interface is to avoid dealing with the buffer allocation on the side of the glob function. It would be better to keep it that way. > 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. I'm not entirely comfortable expanding HOME unconditionally, as it could easily be exploited into a vulnerability for any privileged (e.g. setuid) process. Thank you very much for your comments; I will try to come up with a better version later today.