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 21552 invoked from network); 12 Nov 2020 20:47:12 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 12 Nov 2020 20:47:12 -0000 Received: (qmail 17569 invoked by uid 550); 12 Nov 2020 20:47:07 -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 17551 invoked from network); 12 Nov 2020 20:47:07 -0000 X-Virus-Scanned: Debian amavisd-new at disroot.org Content-Transfer-Encoding: quoted-printable DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=disroot.org; s=mail; t=1605214014; bh=U2h0HF2o8NJhSrt36EACQe/7/Yc7o86eBLbzfC3t0Ys=; h=Subject:From:To:Date:In-Reply-To; b=cvy5bl43U9bj39I2Fyo3hsJvftvsPNRSPq5cIR5QviUjK1RxEjtPew4zPrRIAiuys DN2uxAs25H0x1+Sjl/A2hqT/DriFSPd56tZJ/LjiReKGywlJghrLEVmqBADdMj2OQm nzg9hosgnOiL02o3Bnk1ppLsvZ2zX72hdOtAkMk/UegrvPweFkhzUE5b+KJITwmh8P 7BPNlQpvKKXUZTGI+M0hqsngks50BSCAJ7kt2DA5ZIo4ZlwU1wbttl8ECC7GNoUBNc vlzFOTwbEqb0JbJt9pDwiRa8JAMobKp2CiYwxtMGAG87FQZYF4DCb99pFFOHsfYSmY WxJh3RE4yLw9Q== Content-Type: text/plain; charset=UTF-8 From: =?utf-8?q?=C3=89rico_Nogueira?= To: , Date: Thu, 12 Nov 2020 17:41:45 -0300 Message-Id: In-Reply-To: <20201112200408.GT534@brightrain.aerifal.cx> Subject: Re: [musl] [PATCH] fix segfault in lutimes when tv argument is NULL On Thu Nov 12, 2020 at 12:04 PM -03, Rich Felker wrote: > On Thu, Nov 12, 2020 at 04:43:04PM -0300, =C3=89rico Nogueira wrote: > > On Thu Nov 12, 2020 at 5:32 PM -03, Markus Wichmann wrote: > > > On Thu, Nov 12, 2020 at 03:43:27PM -0300, =C3=89rico Nogueira wrote: > > > > From: =C3=89rico Rolim > > > > > > > > calling lutimes with tv=3D0 is valid if the applications wants to s= et the > > > > timestamps to the current time. short-circuit the function to call > > > > utimensat with times=3D0 directly if tv =3D=3D 0. > > > > --- > > > > > > > > Bug reported on IRC by nmeum > > > > > > > > src/legacy/lutimes.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c > > > > index 2e5502d1..22176230 100644 > > > > --- a/src/legacy/lutimes.c > > > > +++ b/src/legacy/lutimes.c > > > > @@ -5,6 +5,7 @@ > > > > > > > > int lutimes(const char *filename, const struct timeval tv[2]) > > > > { > > > > + if (!tv) return utimensat(AT_FDCWD, filename, 0, AT_SYMLINK_NOFOL= LOW); > > > > struct timespec times[2]; > > > > times[0].tv_sec =3D tv[0].tv_sec; > > > > times[0].tv_nsec =3D tv[0].tv_usec * 1000; > > > > -- > > > > 2.29.2 > > > > > > > > > > Deja vu. We had a similar discussion in early March. The most recent > > > e-mail in that thread stated that the patch "might be correct as-is." > > > Though that patch did attempt to filter out invalid inputs as well. I > > > had pointed out that the only spec available for lutimes does state t= hat > > > it should act like utimes(), and utimes() does allow for NULL inputs, > > > but there was no reply. And no follow-up from the OP, either. > > > > > > Ciao, > > > Markus > >=20 > > For reference, that thread starts at > > https://www.openwall.com/lists/musl/2020/03/01/1 > >=20 > > I based myself off of the futime() implementation, so both functions > > have basically the same look / control flow now (except that futimes() > > has the `struct timespec times[2]` declaration before the null check, > > which I can fix in a v2, if necessary). Since it's a legacy function, I > > didn't think it would be necessary to complicate matters further. > >=20 > > Re. checking the input values beyond a NULL check, futime() currently > > doesn't do it, so for consistency's sake I think it would only make > > sense to add that verification if it was added to futime() as well. Tha= t > > said, I believe any verification should be left to utimensat(), which > > seems to be called by most functions in the utimes family. > > If validation is to be done, it can't be left to utimensat because the > overflow already happened when converting from timeval to timespec. Indeed, as explained in [1] the important check is for overflow, not for negative values. - [1] https://www.openwall.com/lists/musl/2020/03/01/2 > > I don't think I'm opposed to omitting any validation, but I would like > to avoid the duplication of the utimensat call by doing something like > putting the conversion inside if (tv) { } then doing tv ? times : 0 > for the argument. It's not a big deal (the compiler probably compiles > it to the same, or at least hopefully) but it does avoid duplicating > knowledge like the flag to pass in two places. I hadn't thought about the duplication of the information. Sending a v2 with this fixed. > > Rich