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

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 11598 bytes --]

On Thu, May 12, 2022 at 10:08:37AM -0400, Rich Felker wrote:
> On Thu, Jan 13, 2022 at 06:53:19PM +0000, Alyssa Ross wrote:
> > Rich Felker <dalias@libc.org> writes:
> >
> > > On Thu, Jan 13, 2022 at 04:30:18PM +0000, Alyssa Ross wrote:
> > >> Hi Rich, thanks for following up on this.
> > >>
> > >> Rich Felker <dalias@libc.org> writes:
> > >>
> > >> > 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?
> > >>
> > >> Because I only check for the first field being present, a lot of
> > >> nonsensical lines will be accepted.
> > >>
> > >> As I said in the patch commentary:
> > >>
> > >> > After this change, only the first field is
> > >> > required, which matches Glibc's behaviour.
> > >> >
> > >> > [snip]
> > >> >
> > >> > 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.
> > >>
> > >> It probably would make more sense to check that the four fields the man
> > >> pages implies are required are all there, by making the change I
> > >> suggested in the commentary, at least until somebody complains about
> > >> their two-field fstab being accepted by Glibc and not Musl.
> > >>
> > >> > Indeed it also seems to be skipping empty lines, contrary to what you
> > >> > said in another message:
> > >> >
> > >> >>  • Empty lines should be skipped.
> > >>
> > >> Yes, it looks like I was mistaken before when I thought that Musl didn't
> > >> properly handle comments and empty lines.  Looking back, it seems that
> > >> the tests I was running were against mntent files with only four fields,
> > >> so the parsing failures I was seeing were because of that, not because
> > >> of an issue in the comment or empty line handling.
> > >>
> > >> My patch does (inadventently) change the behaviour of empty line
> > >> handling.  We should leave the current behaviour of skipping over empty
> > >> lines as is.  Suggested fix at the end of this message.
> > >>
> > >> > 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?
> > >>
> > >> I think it would clearer to have an explicit check that the last
> > >> mandatory field is set, like I currently do with the mnt_fsname check at
> > >> the end of the function.  I don't particularly mind how many fields are
> > >> mandatory, as long as its four or fewer so Musl's behaviour follows the
> > >> fstab format described in the man page.
> > >>
> > >> So overall my proposed revisions would be the following.  There's a
> > >> change to move to the next line if the current one is empty, and a
> > >> change to ensure the first four fields are all present.  (If you decide
> > >> you'd like the fourth field to also be optional, we can just change
> > >> mnt_opts to mnt_type in that check.)  It's been a long time since I last
> > >> this code, btw, so I hope I'm not missing anything around the empty line
> > >> check.  I'd be happy to put together a revised series, with these
> > >> changes and a corresponding change to my libc-test patch, if you'd like.
> > >>
> > >> diff --git i/src/misc/mntent.c w/src/misc/mntent.c
> > >> index 169e9789..7782cb10 100644
> > >> --- i/src/misc/mntent.c
> > >> +++ w/src/misc/mntent.c
> > >> @@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> > >>  			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]] == '#');
> > >> +	} while (linebuf[0] == '\n' || linebuf[n[0]] == '#');
> > >>
> > >>  	linebuf[n[1]] = 0;
> > >>  	linebuf[n[3]] = 0;
> > >> @@ -59,7 +59,7 @@ 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)
> > >> +	if (!*mnt->mnt_opts)
> > >>  		return 0;
> > >>
> > >>  	return mnt;
> > >
> > > What I still don't like here is that this changes the behavior on
> > > something that's not a valid record (in whatever sense we define that)
> > > from continuing the loop looking for the next one, to returning a null
> > > pointer with no indication of what the error was. Treating empty lines
> > > as an error (rather than continuing) was just one special case of
> > > that. For an analogy, see how the pwd/grp functions work. Something
> > > malformed in the file is just "not a record" and search continues for
> > > the next valid record (if any) rather than giving the caller a
> > > non-actionable (since this is assumed to be a sort of
> > > trusted/authoritative data outside the application's control) error.
>
> Just getting back to this now after the release.. Apologies for the
> long delay.
>
> > Okay, that makes sense.  So it seems like our choices when faced with an
> > invalid record are:
> >
> >  • Skip it and move on to the next one, as you've proposed here; or
> >
> >  • Try to fill in as many fields of the mntent structure as possible,
> >    and return successfully, like Glibc does.
> >
> > If we go with the first option, as you've proposed, do you think the
> > difference in behaviour from Glibc would be an issue?
>
> I think the latter is better. If I remember correctly what we'd found,
> glibc hardly has any condition on what's mandatory (just first field?
> or first and second?) so I think the condition would be something
> like:
>
> 		...
> 	} while (linebuf[n[0]] == '#' || n[1]==len);
>
> or similar, possibly replacing 1 with 3 if 2 fields are mandatory,
> etc.

I'm pretty sure it was just the first field that's mandatory with Glibc.

> > (I'm mostly thinking of the case where a record doesn't have enough
> > fields here.  There are also the cases where a record has too many
> > fields, or when fields 5 and 6 are numeric.  I haven't looked into how
> > Glibc handles those yet.)
>
> A few other changes I think should be made:
>
> - The two numeric fields should be read into temporaries initialized
>   with default values so that it doesn't matter if they're read or
>   not.

Does that make a difference?  We already explicitly zero them at the
start of the function.  (Maybe I'm forgetting some scanf arcana — it's
been a while since I've thought about this.)

> - The n[] array should be changed to size_t[] and the %n's to %zn's.
>   This should actually be done as a separate change, as it's a fix for
>   a bug overlooked when 05973dc3bbc1aca9b3c8347de6879ed72147ab3b made
>   the buffer length potentially longer than INT_MAX.

Yes.  IIRC I half did this in my original attempt at this patch, but
didn't change the format specifiers.

> - There's also an independent bug in hasmntent that was reported a
>   long time ago then lost: it will return false positives when one
>   mntopt name is a substring of another. strstr is just not the right
>   operation here, at least not without added logic to ensure matching
>   on a whole option boundary. This is a separate issue that calls for
>   a separate patch though, not a blocker on the patch under discussion
>   here.
>
> Rich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]