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.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 11546 invoked from network); 13 Jan 2022 16:31:02 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 13 Jan 2022 16:31:02 -0000 Received: (qmail 5455 invoked by uid 550); 13 Jan 2022 16:30:59 -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 5418 invoked from network); 13 Jan 2022 16:30:58 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alyssa.is; h= from:to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=fm2; bh=sN4bz9vC53Ej9Dvm/Xx1dNlVYQ VRalyqvfg4na1m0og=; b=E/Kd4XSHS42UXVc6KelqPvZppOzy112rbUdEVt2XlQ RjJXYJmji/0pE7+vsGGgnhBCbgHz2HyNnpUGOe995q/gdgqbA+fYhiJzS/aIoU2d rAvurIdpxLr+10bNdT71yPGNijkO0QT9quHJRBnYoMBty1Demu+zEjo3QsMbO27O HEwITWu7ulZnpiSf01AnWKipeuwB+xwOUMBqjPfOp0SxzDBCFKDNIv4P7sNxhVkV ateFVYxqvHYlG3oNfW37h8NHE/SwBcNI402+HWpX6gk9I/z3x9D2VN/snr9hr6vV IoSDIpTqfrbYEfETQPKF8R2atxTbbNeRupSqdQ8XLKZw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=sN4bz9 vC53Ej9Dvm/Xx1dNlVYQVRalyqvfg4na1m0og=; b=YIv3FcdRpIVgJ50oOBorNU zvmj3fwlqdp1e/BNibNYIWpvCJF8KHvGdj2EnrgXSqaWQSeuV/KRFml2J8tFNLtq x+9QZ8yiaIHZKAFoxxOgrV4WIyw6DChaWMQmzrx8llaRkGUAO26y9QI8zPcfFLrB DJlxSYuz4DSe/Tl0EKPnig7/jVmCuV+dGhFkfS/woKdAcz0UKdhhG6k8DE5IC5sU FuizqSZlni1R6Cf6p59aU+irSvScppTi+Eca4da8uOCkOCQgds6xNdthiEv5O+cD kWSo5vImy4DGXZjUK3XfOswIjLjzlYQUC1WUFFKFqkkZQRuSAm3WlVtqfKd9cMAA == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrtdefgdekkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecunecujfgurhephffvufgjfhffkfggtgesghdtreertd dtjeenucfhrhhomheptehlhihsshgrucftohhsshcuoehhihesrghlhihsshgrrdhisheq necuggftrfgrthhtvghrnhepveelfeevffeuudfgheehheevfeefgfevfeetudehvefhvd eltdfgheegjeeitdefnecuffhomhgrihhnpehmnhhtvghnthdrtgifnecuvehluhhsthgv rhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhephhhisegrlhihshhsrgdrih hs X-ME-Proxy: From: Alyssa Ross To: Rich Felker Cc: musl@lists.openwall.com In-Reply-To: <20220109031819.GO7074@brightrain.aerifal.cx> References: <20210915221155.3977763-1-hi@alyssa.is> <20210915221155.3977763-4-hi@alyssa.is> <20210920042140.GT13220@brightrain.aerifal.cx> <20220109031819.GO7074@brightrain.aerifal.cx> Date: Thu, 13 Jan 2022 16:30:18 +0000 Message-ID: <878rvj1tut.fsf@alyssa.is> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Subject: Re: [musl] [PATCH musl v2 3/3] mntent: fix parsing lines with optional fields --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 this >> > wasn't accepted by Musl. After this change, only the first field is >> > required, which matches Glibc's behaviour. >> >=20 >> > 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. >> > --- >> >=20 >> > 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. >> >=20 >> > v2: don't change n from int to size_t >> >=20 >> > src/misc/mntent.c | 18 +++++++++++++----- >> > 1 file changed, 13 insertions(+), 5 deletions(-) >> >=20 >> > 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) >> >=20=20 >> > 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; >> >=20=20 >> > 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 '#'); >> >=20=20 >> > linebuf[n[1]] =3D 0; >> > linebuf[n[3]] =3D 0; >> > @@ -54,6 +60,9 @@ struct mntent *getmntent_r(FILE *f, struct mntent *m= nt, char *linebuf, int bufle >> > mnt->mnt_type =3D linebuf+n[4]; >> > mnt->mnt_opts =3D linebuf+n[6]; >> >=20=20 >> > + if (!*mnt->mnt_fsname) >> > + return 0; >> > + >> > return mnt; >> > } >>=20 >> 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: > >> =E2=80=A2 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]]=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 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 =2D-- i/src/misc/mntent.c +++ w/src/misc/mntent.c @@ -47,7 +47,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, c= har *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; =2D } while (linebuf[n[0]] =3D=3D '#'); + } while (linebuf[0] =3D=3D '\n' || linebuf[n[0]] =3D=3D '#'); =20 linebuf[n[1]] =3D 0; linebuf[n[3]] =3D 0; @@ -59,7 +59,7 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, c= har *linebuf, int bufle mnt->mnt_type =3D linebuf+n[4]; mnt->mnt_opts =3D linebuf+n[6]; =20 =2D if (!*mnt->mnt_fsname) + if (!*mnt->mnt_opts) return 0; =20 return mnt; --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEH9wgcxqlHM/ARR3h+dvtSFmyccAFAmHgU50ACgkQ+dvtSFmy ccBeiRAAgEIPF3ZERMogbWTG2afEgBLuVCx63PkmFe5Flrhcp+w2KU+78aZ8BaBK oEzR8v0S13odgT2ksj1sRtyWAgt1GTD8GT6QjPHnvBrFP7X5tTQ+NNXv9rJVIwXc nVEb3NUGtxoX1aBTY7OqopzSkwP9WYU8ZjRwKgSEUAuH+qVxBaQDIrmA8EFUNJxC 0xAfW/SbjCLBWbz2FMWT/XOhm8coYEmHbZvs6kdVegyO9Fd7LKkCgaNBmNT4ZO1v /UAKQFXb1QBMfCtUaTFUvug5eNxd7d7xWaMxAFIfFklxD2O8l2dqeiCz9Gg3jbxp P9N+CnKW1msV3/9Be64Dds/D2bMfLR8D/DMhmq/ZxnYX7VzGoHeZfVhdEAGXd5/m FF1rVzEn4JTRtZCOoPXAvcLtSztlpFu2nQGm3417OPQzH0I4cK/7JTxl1yn1l9ql btkKLULe4zdv2+YyJfYwtl2tml8PJ7obWXEgfBm2i1wAYA3nxRTRpIocf5Rym0jd NJWCP58rxiwZkzowpvGN1Wirp4kJQULpC3OtCmp1Nq8anLQHTaiCqVuZdh6YTBoK aZpWEvHuVBVPTCyDIDAaNqbEsNH3WXnacJngEro9D2kz39eTtoP43gxavuKhmr1K fijA4tGB7CD2fYr3HZK6wJq7EEFZa8ilnlZsDMkuuCpCq7gLGpk= =xg28 -----END PGP SIGNATURE----- --=-=-=--