Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields

[Rich Felker <dalias@libc.org>]

On Mon, Sep 20, 2021 at 12:21:41AM -0400, Rich Felker wrote:
> On Wed, Sep 15, 2021 at 10:11:55PM +0000, Alyssa Ross wrote:
> > According to fstab(5), the last two fields are optional, but this
> > wasn't accepted by Musl.  After this change, only the first field is
> > required, which matches Glibc's behaviour.
> > 
> > Using sscanf as before, it would have been impossible to differentiate
> > between 0 fields and 4 fields, because sscanf would have returned 0 in
> > both cases due to the use of assignment suppression and %n for the
> > string fields (which is important to avoid copying any strings).  So
> > instead, before calling sscanf, initialize every string to the empty
> > string, and then we can check which strings are empty afterwards to
> > know how many fields were matched.
> > ---
> > 
> > We could also be stricter about it, and enforce that the first four
> > fields are present, since the man page says only the last two are
> > optional.  Doing that would be a simple change of checking for the
> > presence of mnt_opts instead of mnt_fsname at the end of my patch.
> > 
> > v2: don't change n from int to size_t
> > 
> >  src/misc/mntent.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> > index eabb8200..238a0efd 100644
> > --- a/src/misc/mntent.c
> > +++ b/src/misc/mntent.c
> > @@ -21,7 +21,8 @@ int endmntent(FILE *f)
> >  
> >  struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int buflen)
> >  {
> > -	int cnt, n[8], use_internal = (linebuf == SENTINEL);
> > +	int n[8], use_internal = (linebuf == SENTINEL);
> > +	size_t len, i;
> >  
> >  	mnt->mnt_freq = 0;
> >  	mnt->mnt_passno = 0;
> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >  			errno = ERANGE;
> >  			return 0;
> >  		}
> > -		cnt = sscanf(linebuf, " %n%*s%n %n%*s%n %n%*s%n %n%*s%n %d %d",
> > -			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> > -			&mnt->mnt_freq, &mnt->mnt_passno);
> > -	} while (cnt < 2 || linebuf[n[0]] == '#');
> > +
> > +		len = strlen(linebuf);
> > +		for (i = 0; i < sizeof n / sizeof *n; i++) n[i] = len;
> > +		if (sscanf(linebuf, " %n%*s%n %n%*s%n %n%*s%n %n%*s%n %d %d",
> > +			n, n+1, n+2, n+3, n+4, n+5, n+6, n+7,
> > +			&mnt->mnt_freq, &mnt->mnt_passno) == EOF && ferror(f))
> > +			return 0;
> > +	} while (linebuf[n[0]] == '#');
> >  
> >  	linebuf[n[1]] = 0;
> >  	linebuf[n[3]] = 0;
> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> >  	mnt->mnt_type = linebuf+n[4];
> >  	mnt->mnt_opts = linebuf+n[6];
> >  
> > +	if (!*mnt->mnt_fsname)
> > +		return 0;
> > +
> >  	return mnt;
> >  }
> 
> It looks like your patch changes the behavior for malformed lines from
> skipping them (and continuing to search for the next valid line) to
> returning 0. Is that intentional? Maybe it's better; I'm not sure. But
> won't it even cause blank lines to return 0?

Indeed it also seems to be skipping empty lines, contrary to what you
said in another message:

>  • Empty lines should be skipped.

Do you have a preference on how to proceed? We could add back a
condition to the while loop, something like linebuf[n[0]]=='#' ||
n[6]==len (i.e. skip lines with too few fields, possibly using a
different number instead of 6 if more appropriate). Or we could do
what I suggested before:

> A less invasive change might be adding "%1[ \t\n\v\f\r]" and a dummy
> char* argument to collect the value before the " %d %d". Then you can
> check for cnt<1. But I'm not sure even the 4th field should be
> mandatory. This same apprach could be used to make just 3 mandatory if
> desired though.

Thoughts?

Rich