From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.1 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 6506 invoked from network); 12 May 2022 18:02:54 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 12 May 2022 18:02:54 -0000 Received: (qmail 20121 invoked by uid 550); 12 May 2022 18:02:51 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 20083 invoked from network); 12 May 2022 18:02:50 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h=cc :cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm3; t=1652378558; x=1652464958; bh=xIHxScrBxa nulgayftq9VUkcKL7/5xdNq8+EGD8mPUc=; b=nt5CTCaXxowuBqOR59ScFx9Lcm RJejFnOcAfo6EH7L3fcwa96ppSBHjMKH/uaKcPEPklcrsKaFIfpj+SF1SlBpUu2/ hxDgMVNE2v34lacq4yDiSj4ekcximFMRt/rArevwwV7o3XA2JFf0RBCIo3hltxgx BSdWGgSPaz5qVwU5xdF3un45752eegNZqM+uTwwoTQZlWigxP5NLj1hcOg4L66D9 PI/kHnKaLGkxyH/bfqHP+XEA6w3UzXTZIbD2kA6ATMhcnk1xJh6D+tmpexY/dNHs Wn4dJvlcVh+RESI8+Uv5u36ao/kuKBHeSRaotuo83ewXiu7sd9YXz3hXdeIA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1652378558; x= 1652464958; bh=xIHxScrBxanulgayftq9VUkcKL7/5xdNq8+EGD8mPUc=; b=j 6OdPmBo3xbHu/EaKtxKijj5ieIY1V3gLYwH/ckazgLiI4s7ZmovMaSTq934nLN+b 4dbrnsWi8IHQjT0TU+bd+Ah2Sl9Z1p5wpLUKI0CeiQSkiMGMYCy55D1D37Ke9HZz JOLhGf2EuJGI0axMxwr3mL448fribY8WwEl/kODsgAoyE+FOK1D06MhdxQxbxLYC dW0o/xmqBe9Mu0PNbOdBT/KM+jbyZZtnkeIjQk6JCOhHXJjDLIZyyueRvfSR8PiP LtiRD+XOWvccYw8nc8uPyMoXhZkN+DL322kEk+qN2xjUY998xWsJZJn8nB5mmKXW JjyeSHcEUTk6xWwxSGrhg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrgeejgdduudejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpeffhffvvefukfhfgggtuggjsehgtd erredttdejnecuhfhrohhmpeetlhihshhsrgcutfhoshhsuceohhhisegrlhihshhsrgdr ihhsqeenucggtffrrghtthgvrhhnpeehgeelvdeifeegfedtudehkeffieekhfejhffhge ejiefhhfevhfetueefgfffgfenucffohhmrghinhepmhhnthgvnhhtrdgtfienucevlhhu shhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehqhihlihhsshesvg hvvgdrqhihlhhishhsrdhnvght X-ME-Proxy: Date: Thu, 12 May 2022 18:02:35 +0000 From: Alyssa Ross To: Rich Felker Cc: musl@lists.openwall.com Message-ID: <20220512180235.5uadgd4iyxtdr37p@eve> References: <20210915221155.3977763-1-hi@alyssa.is> <20210915221155.3977763-4-hi@alyssa.is> <20210920042140.GT13220@brightrain.aerifal.cx> <20220109031819.GO7074@brightrain.aerifal.cx> <878rvj1tut.fsf@alyssa.is> <20220113174037.GA7074@brightrain.aerifal.cx> <875yqn1n8g.fsf@alyssa.is> <20220512140835.GJ7074@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="h6iycgahf7kizb4c" Content-Disposition: inline In-Reply-To: <20220512140835.GJ7074@brightrain.aerifal.cx> Subject: Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields --h6iycgahf7kizb4c Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 writes: > > > > > On Thu, Jan 13, 2022 at 04:30:18PM +0000, Alyssa Ross wrote: > > >> Hi Rich, thanks for following up on this. > > >> > > >> Rich Felker 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 th= is > > >> >> > wasn't accepted by Musl. After this change, only the first fie= ld is > > >> >> > required, which matches Glibc's behaviour. > > >> >> > > > >> >> > Using sscanf as before, it would have been impossible to differ= entiate > > >> >> > between 0 fields and 4 fields, because sscanf would have return= ed 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)= =2E So > > >> >> > instead, before calling sscanf, initialize every string to the = empty > > >> >> > string, and then we can check which strings are empty afterward= s 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 a= re > > >> >> > 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 pat= ch. > > >> >> > > > >> >> > 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 =3D (linebuf =3D=3D SENTINEL); > > >> >> > + int n[8], use_internal =3D (linebuf =3D=3D SENTINEL); > > >> >> > + size_t len, i; > > >> >> > > > >> >> > mnt->mnt_freq =3D 0; > > >> >> > mnt->mnt_passno =3D 0; > > >> >> > @@ -39,10 +40,14 @@ struct mntent *getmntent_r(FILE *f, struct = mntent *mnt, char *linebuf, int bufle > > >> >> > errno =3D ERANGE; > > >> >> > return 0; > > >> >> > } > > >> >> > - cnt =3D 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]] =3D=3D '#'); > > >> >> > + > > >> >> > + len =3D strlen(linebuf); > > >> >> > + for (i =3D 0; i < sizeof n / sizeof *n; i++) n[i] =3D 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) =3D=3D EOF && ferror(f)) > > >> >> > + return 0; > > >> >> > + } while (linebuf[n[0]] =3D=3D '#'); > > >> >> > > > >> >> > linebuf[n[1]] =3D 0; > > >> >> > linebuf[n[3]] =3D 0; > > >> >> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mn= tent *mnt, char *linebuf, int bufle > > >> >> > mnt->mnt_type =3D linebuf+n[4]; > > >> >> > mnt->mnt_opts =3D 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= =2E 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: > > >> > > > >> >> =E2=80=A2 Empty lines should be skipped. > > >> > > >> Yes, it looks like I was mistaken before when I thought that Musl di= dn't > > >> properly handle comments and empty lines. Looking back, it seems th= at > > >> the tests I was running were against mntent files with only four fie= lds, > > >> so the parsing failures I was seeing were because of that, not becau= se > > >> 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 em= pty > > >> 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]]=3D=3D'#'= || > > >> > n[6]=3D=3Dlen (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 du= mmy > > >> >> 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 mandato= ry 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 chec= k 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 dec= ide > > >> 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 l= ike. > > >> > > >> 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) =3D=3D EOF && ferror(f)) > > >> return 0; > > >> - } while (linebuf[n[0]] =3D=3D '#'); > > >> + } while (linebuf[0] =3D=3D '\n' || linebuf[n[0]] =3D=3D '#'); > > >> > > >> linebuf[n[1]] =3D 0; > > >> linebuf[n[3]] =3D 0; > > >> @@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent = *mnt, char *linebuf, int bufle > > >> mnt->mnt_type =3D linebuf+n[4]; > > >> mnt->mnt_opts =3D 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: > > > > =E2=80=A2 Skip it and move on to the next one, as you've proposed here= ; or > > > > =E2=80=A2 Try to fill in as many fields of the mntent structure as pos= sible, > > 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]] =3D=3D '#' || n[1]=3D=3Dlen); > > 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 =E2=80=94 i= t'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 --h6iycgahf7kizb4c Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEH9wgcxqlHM/ARR3h+dvtSFmyccAFAmJ9S7MACgkQ+dvtSFmy ccDGCA//dXxbq53KUqMIRyOWDGl/DPaBrk6kQ87hQzrKukg+gAO3lZCl0VBXq2IS gPwjO/zeZW1ykXyuJpf8vyQ2Lacs5aHPOyZhmxlz6pAMl+31tXZ862wM+SPPVdfK cupqzEwog3Oq+fC0is3KSw41GZX5ucz3fiSnpkcXd5ItzY5tUy1A7eNuyVTE+BXQ essZnHNQmN+mG+IzSdVzmuuX14SOvcAcrbf9eVgG0ID8trzLMFhGSBU8irIm4bEZ PWdcfoITM6UKoxXm0X5nfDYeaXNHBKfjA2ajV6e5/QWkcbTj2IVGDeFhn/XhkToU o8hA5yOhCvSX9qTO2OJ5vCXvORV0VigKcQw/xMwx8WFw+4HZEKDeUpUFAWCjyA9e EsW69jxk6m05c8RdF8vlUUYrRx/1RWBjCzyoWTcNRbZv/caCbsF8H3q2J62KA83M mLdcwafhU20Q27AFXd8OX6rzAQWTeTm8vwtRAnhBGDeAa3WgRgeChYhvVUVBod6v 1j5u6YF/JyQVzg8ymnSEgSNHjau14boY7iVCyGrM4By55GCNCp9DXHaoecCNxNqA bjIImJXkeZhe7a1EVvOwbkPp09nSCEMr0aCa6ywHsvuikaF/bgIKXMobt2U//8hf Is0RWJKjxIm1qzOx+k9DHUvM5Jfe8DcWVrmpfHyh8ntDsO6QvTc= =BUUv -----END PGP SIGNATURE----- --h6iycgahf7kizb4c--