[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201112200408.GT534@brightrain.aerifal.cx>
Date: Thu, 12 Nov 2020 15:04:08 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [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 <ericonr@...root.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.
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
