Re: [musl] [PATCH] fix segfault in lutimes when tv argument is NULL

["Érico Nogueira" <ericonr@disroot.org>]

On Thu Nov 12, 2020 at 12:04 PM -03, Rich Felker wrote:
> 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 <ericonr@disroot.org>
> > > >
> > > > 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.

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