mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] fix segfault in lutimes when tv argument is NULL
@ 2020-11-12 18:43 Érico Nogueira
  2020-11-12 19:32 ` Markus Wichmann
  0 siblings, 1 reply; 5+ messages in thread
From: Érico Nogueira @ 2020-11-12 18:43 UTC (permalink / raw)
  To: musl; +Cc: Érico Rolim

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [musl] [PATCH] fix segfault in lutimes when tv argument is NULL
  2020-11-12 18:43 [musl] [PATCH] fix segfault in lutimes when tv argument is NULL Érico Nogueira
@ 2020-11-12 19:32 ` Markus Wichmann
  2020-11-12 19:43   ` Érico Nogueira
  0 siblings, 1 reply; 5+ messages in thread
From: Markus Wichmann @ 2020-11-12 19:32 UTC (permalink / raw)
  To: musl

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [musl] [PATCH] fix segfault in lutimes when tv argument is NULL
  2020-11-12 19:32 ` Markus Wichmann
@ 2020-11-12 19:43   ` Érico Nogueira
  2020-11-12 20:04     ` Rich Felker
  0 siblings, 1 reply; 5+ messages in thread
From: Érico Nogueira @ 2020-11-12 19:43 UTC (permalink / raw)
  To: musl, musl

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.

Cheers,
Érico

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [musl] [PATCH] fix segfault in lutimes when tv argument is NULL
  2020-11-12 19:43   ` Érico Nogueira
@ 2020-11-12 20:04     ` Rich Felker
  2020-11-12 20:41       ` Érico Nogueira
  0 siblings, 1 reply; 5+ messages in thread
From: Rich Felker @ 2020-11-12 20:04 UTC (permalink / raw)
  To: musl

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.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [musl] [PATCH] fix segfault in lutimes when tv argument is NULL
  2020-11-12 20:04     ` Rich Felker
@ 2020-11-12 20:41       ` Érico Nogueira
  0 siblings, 0 replies; 5+ messages in thread
From: Érico Nogueira @ 2020-11-12 20:41 UTC (permalink / raw)
  To: musl, musl

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-12 20:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 18:43 [musl] [PATCH] fix segfault in lutimes when tv argument is NULL Érico Nogueira
2020-11-12 19:32 ` Markus Wichmann
2020-11-12 19:43   ` Érico Nogueira
2020-11-12 20:04     ` Rich Felker
2020-11-12 20:41       ` Érico Nogueira

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).