Re: [PATCH] glob: implement GLOB_TILDE

[Ismael Luceno <ismael@iodev.co.uk>]

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 <ismael@iodev.co.uk>
> > ---
> >  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 <stdlib.h>
> >  #include <errno.h>
> >  #include <stddef.h>
> > +#include <unistd.h>
> > +#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.