* [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).