* [musl] [PATCH] musl: lutimes: Add checks for input parameters @ 2020-03-01 6:57 Liu Jie 2020-03-01 7:17 ` Rich Felker 2020-03-01 8:37 ` Markus Wichmann 0 siblings, 2 replies; 6+ messages in thread From: Liu Jie @ 2020-03-01 6:57 UTC (permalink / raw) To: musl; +Cc: yunlong.song, liujie1 For the input parameter struct timeval tv, need to determine whether it is invalid inputs. Signed-off-by: Liu Jie <liujie1@huawei.com> --- src/legacy/lutimes.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c index 2e5502d1..6e7e1be3 100644 --- a/src/legacy/lutimes.c +++ b/src/legacy/lutimes.c @@ -2,13 +2,22 @@ #include <sys/stat.h> #include <sys/time.h> #include <fcntl.h> +#include <stdio.h> +#include <errno.h> int lutimes(const char *filename, const struct timeval tv[2]) { struct timespec times[2]; - times[0].tv_sec = tv[0].tv_sec; - times[0].tv_nsec = tv[0].tv_usec * 1000; - times[1].tv_sec = tv[1].tv_sec; - times[1].tv_nsec = tv[1].tv_usec * 1000; - return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW); + if (tv != NULL) { + if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 || + tv[1].tv_sec < 0 || tv[1].tv_usec < 0) { + errno = EINVAL; + return -1; + } + times[0].tv_sec = tv[0].tv_sec; + times[0].tv_nsec = tv[0].tv_usec * 1000; + times[1].tv_sec = tv[1].tv_sec; + times[1].tv_nsec = tv[1].tv_usec * 1000; + } + return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW); } -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters 2020-03-01 6:57 [musl] [PATCH] musl: lutimes: Add checks for input parameters Liu Jie @ 2020-03-01 7:17 ` Rich Felker 2020-03-01 20:37 ` Samuel Holland 2020-03-01 8:37 ` Markus Wichmann 1 sibling, 1 reply; 6+ messages in thread From: Rich Felker @ 2020-03-01 7:17 UTC (permalink / raw) To: musl On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote: > For the input parameter struct timeval tv, need to > determine whether it is invalid inputs. > > Signed-off-by: Liu Jie <liujie1@huawei.com> > --- > src/legacy/lutimes.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c > index 2e5502d1..6e7e1be3 100644 > --- a/src/legacy/lutimes.c > +++ b/src/legacy/lutimes.c > @@ -2,13 +2,22 @@ > #include <sys/stat.h> > #include <sys/time.h> > #include <fcntl.h> > +#include <stdio.h> > +#include <errno.h> > > int lutimes(const char *filename, const struct timeval tv[2]) > { > struct timespec times[2]; > - times[0].tv_sec = tv[0].tv_sec; > - times[0].tv_nsec = tv[0].tv_usec * 1000; > - times[1].tv_sec = tv[1].tv_sec; > - times[1].tv_nsec = tv[1].tv_usec * 1000; > - return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW); > + if (tv != NULL) { > + if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 || > + tv[1].tv_sec < 0 || tv[1].tv_usec < 0) { > + errno = EINVAL; > + return -1; > + } > + times[0].tv_sec = tv[0].tv_sec; > + times[0].tv_nsec = tv[0].tv_usec * 1000; > + times[1].tv_sec = tv[1].tv_sec; > + times[1].tv_nsec = tv[1].tv_usec * 1000; > + } > + return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW); > } > -- > 2.17.1 This patch causes uninitialized timespecs to be used if a null pointer is passed, silently corrupting data. If there is any historical documented precedent for this function accepting a null pointer and doing something meaningful, then the patch needs to do whatever that meaningful thing is rather than usign uninitialized data. If not, the preferred behavior is the current behavior: to crash so that the usage error is caught. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters 2020-03-01 7:17 ` Rich Felker @ 2020-03-01 20:37 ` Samuel Holland 2020-03-01 20:39 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Samuel Holland @ 2020-03-01 20:37 UTC (permalink / raw) To: musl, Rich Felker On 3/1/20 1:17 AM, Rich Felker wrote: > On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote: >> For the input parameter struct timeval tv, need to >> determine whether it is invalid inputs. >> >> Signed-off-by: Liu Jie <liujie1@huawei.com> >> --- >> src/legacy/lutimes.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c >> index 2e5502d1..6e7e1be3 100644 >> --- a/src/legacy/lutimes.c >> +++ b/src/legacy/lutimes.c >> @@ -2,13 +2,22 @@ >> #include <sys/stat.h> >> #include <sys/time.h> >> #include <fcntl.h> >> +#include <stdio.h> >> +#include <errno.h> >> >> int lutimes(const char *filename, const struct timeval tv[2]) >> { >> struct timespec times[2]; >> - times[0].tv_sec = tv[0].tv_sec; >> - times[0].tv_nsec = tv[0].tv_usec * 1000; >> - times[1].tv_sec = tv[1].tv_sec; >> - times[1].tv_nsec = tv[1].tv_usec * 1000; >> - return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW); >> + if (tv != NULL) { >> + if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 || >> + tv[1].tv_sec < 0 || tv[1].tv_usec < 0) { >> + errno = EINVAL; >> + return -1; >> + } >> + times[0].tv_sec = tv[0].tv_sec; >> + times[0].tv_nsec = tv[0].tv_usec * 1000; >> + times[1].tv_sec = tv[1].tv_sec; >> + times[1].tv_nsec = tv[1].tv_usec * 1000; >> + } >> + return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW); >> } >> -- >> 2.17.1 > > This patch causes uninitialized timespecs to be used if a null pointer > is passed, silently corrupting data. If there is any historical > documented precedent for this function accepting a null pointer and > doing something meaningful, then the patch needs to do whatever that > meaningful thing is rather than usign uninitialized data. If not, the > preferred behavior is the current behavior: to crash so that the usage > error is caught. How do you see that uninitialized timespecs are used? times is only passed to utimensat if tv is nonnull, and in that case times is initialized. Regards, Samuel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters 2020-03-01 20:37 ` Samuel Holland @ 2020-03-01 20:39 ` Rich Felker 2020-03-20 19:54 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2020-03-01 20:39 UTC (permalink / raw) To: musl On Sun, Mar 01, 2020 at 02:37:38PM -0600, Samuel Holland wrote: > On 3/1/20 1:17 AM, Rich Felker wrote: > > On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote: > >> For the input parameter struct timeval tv, need to > >> determine whether it is invalid inputs. > >> > >> Signed-off-by: Liu Jie <liujie1@huawei.com> > >> --- > >> src/legacy/lutimes.c | 19 ++++++++++++++----- > >> 1 file changed, 14 insertions(+), 5 deletions(-) > >> > >> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c > >> index 2e5502d1..6e7e1be3 100644 > >> --- a/src/legacy/lutimes.c > >> +++ b/src/legacy/lutimes.c > >> @@ -2,13 +2,22 @@ > >> #include <sys/stat.h> > >> #include <sys/time.h> > >> #include <fcntl.h> > >> +#include <stdio.h> > >> +#include <errno.h> > >> > >> int lutimes(const char *filename, const struct timeval tv[2]) > >> { > >> struct timespec times[2]; > >> - times[0].tv_sec = tv[0].tv_sec; > >> - times[0].tv_nsec = tv[0].tv_usec * 1000; > >> - times[1].tv_sec = tv[1].tv_sec; > >> - times[1].tv_nsec = tv[1].tv_usec * 1000; > >> - return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW); > >> + if (tv != NULL) { > >> + if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 || > >> + tv[1].tv_sec < 0 || tv[1].tv_usec < 0) { > >> + errno = EINVAL; > >> + return -1; > >> + } > >> + times[0].tv_sec = tv[0].tv_sec; > >> + times[0].tv_nsec = tv[0].tv_usec * 1000; > >> + times[1].tv_sec = tv[1].tv_sec; > >> + times[1].tv_nsec = tv[1].tv_usec * 1000; > >> + } > >> + return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW); > >> } > >> -- > >> 2.17.1 > > > > This patch causes uninitialized timespecs to be used if a null pointer > > is passed, silently corrupting data. If there is any historical > > documented precedent for this function accepting a null pointer and > > doing something meaningful, then the patch needs to do whatever that > > meaningful thing is rather than usign uninitialized data. If not, the > > preferred behavior is the current behavior: to crash so that the usage > > error is caught. > > How do you see that uninitialized timespecs are used? times is only passed to > utimensat if tv is nonnull, and in that case times is initialized. Oh, I misread it. In that case it seems like it might be correct as-is. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters 2020-03-01 20:39 ` Rich Felker @ 2020-03-20 19:54 ` Rich Felker 0 siblings, 0 replies; 6+ messages in thread From: Rich Felker @ 2020-03-20 19:54 UTC (permalink / raw) To: musl On Sun, Mar 01, 2020 at 03:39:21PM -0500, Rich Felker wrote: > On Sun, Mar 01, 2020 at 02:37:38PM -0600, Samuel Holland wrote: > > On 3/1/20 1:17 AM, Rich Felker wrote: > > > On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote: > > >> For the input parameter struct timeval tv, need to > > >> determine whether it is invalid inputs. > > >> > > >> Signed-off-by: Liu Jie <liujie1@huawei.com> > > >> --- > > >> src/legacy/lutimes.c | 19 ++++++++++++++----- > > >> 1 file changed, 14 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c > > >> index 2e5502d1..6e7e1be3 100644 > > >> --- a/src/legacy/lutimes.c > > >> +++ b/src/legacy/lutimes.c > > >> @@ -2,13 +2,22 @@ > > >> #include <sys/stat.h> > > >> #include <sys/time.h> > > >> #include <fcntl.h> > > >> +#include <stdio.h> > > >> +#include <errno.h> > > >> > > >> int lutimes(const char *filename, const struct timeval tv[2]) > > >> { > > >> struct timespec times[2]; > > >> - times[0].tv_sec = tv[0].tv_sec; > > >> - times[0].tv_nsec = tv[0].tv_usec * 1000; > > >> - times[1].tv_sec = tv[1].tv_sec; > > >> - times[1].tv_nsec = tv[1].tv_usec * 1000; > > >> - return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW); > > >> + if (tv != NULL) { > > >> + if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 || > > >> + tv[1].tv_sec < 0 || tv[1].tv_usec < 0) { > > >> + errno = EINVAL; > > >> + return -1; > > >> + } > > >> + times[0].tv_sec = tv[0].tv_sec; > > >> + times[0].tv_nsec = tv[0].tv_usec * 1000; > > >> + times[1].tv_sec = tv[1].tv_sec; > > >> + times[1].tv_nsec = tv[1].tv_usec * 1000; > > >> + } > > >> + return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW); > > >> } > > >> -- > > >> 2.17.1 > > > > > > This patch causes uninitialized timespecs to be used if a null pointer > > > is passed, silently corrupting data. If there is any historical > > > documented precedent for this function accepting a null pointer and > > > doing something meaningful, then the patch needs to do whatever that > > > meaningful thing is rather than usign uninitialized data. If not, the > > > preferred behavior is the current behavior: to crash so that the usage > > > error is caught. > > > > How do you see that uninitialized timespecs are used? times is only passed to > > utimensat if tv is nonnull, and in that case times is initialized. > > Oh, I misread it. In that case it seems like it might be correct > as-is. I've looked back into this, and indeed I think the concept of the patch makes sense. The equivalent logic for utimes() is buried in __futimesat, but can't be reused directly because __futimesat doesn't take a flags argument for AT_NOFOLLOW. There are some subtle differences to how they handle error cases, so it'd be nice to figure out exactly what the right behavior should be and get it right both places. Sorry for taking a while to get back to this. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters 2020-03-01 6:57 [musl] [PATCH] musl: lutimes: Add checks for input parameters Liu Jie 2020-03-01 7:17 ` Rich Felker @ 2020-03-01 8:37 ` Markus Wichmann 1 sibling, 0 replies; 6+ messages in thread From: Markus Wichmann @ 2020-03-01 8:37 UTC (permalink / raw) To: musl On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote: > For the input parameter struct timeval tv, need to > determine whether it is invalid inputs. > Why? lutimes() is a Linux-specific function, so the manpage is as close to a specification as you're ever going to get, and it does not specify an EINVAL return. Adding the NULL pointer check, though, is probably justified, given that the manpage states that lutimes() acts "in the same way as utimes(2)" (with an irrelevant exception afterwards), and utimes() allows for a NULL tv input. The kernel itself also checks the input values again. While I usually am in favor of failing faster, in this case I fail to see the benefit. Especially since you're not testing for the one case that could make the kernel accept a timestamp that was invalid on input: An overflowing one. But you don't test for the upper limit. Oh, and the seconds are allowed to be negative. If someone wants to set a timestamp from before 1970, the libc is the wrong place to stop them. If such dates are invalid from your application's perspective, filter that there. Have a nice Sunday, Markus ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-20 19:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-01 6:57 [musl] [PATCH] musl: lutimes: Add checks for input parameters Liu Jie 2020-03-01 7:17 ` Rich Felker 2020-03-01 20:37 ` Samuel Holland 2020-03-01 20:39 ` Rich Felker 2020-03-20 19:54 ` Rich Felker 2020-03-01 8:37 ` Markus Wichmann
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).