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.3 required=5.0 tests=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 16597 invoked from network); 12 Nov 2020 20:04:26 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 12 Nov 2020 20:04:26 -0000 Received: (qmail 28044 invoked by uid 550); 12 Nov 2020 20:04:21 -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 28025 invoked from network); 12 Nov 2020 20:04:20 -0000 Date: Thu, 12 Nov 2020 15:04:08 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20201112200408.GT534@brightrain.aerifal.cx> References: <20201112193207.GC2009@voyager> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] fix segfault in lutimes when tv argument is NULL On Thu, Nov 12, 2020 at 04:43:04PM -0300, Érico 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, Érico Nogueira wrote: > > > From: Érico Rolim > > > > > > calling lutimes with tv=0 is valid if the applications wants to set the > > > timestamps to the current time. short-circuit the function to call > > > utimensat with times=0 directly if tv == 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_NOFOLLOW); > > > struct timespec times[2]; > > > times[0].tv_sec = tv[0].tv_sec; > > > times[0].tv_nsec = 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 that > > 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 > > For reference, that thread starts at > https://www.openwall.com/lists/musl/2020/03/01/1 > > 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. > > 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. That > 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. 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. Rich