Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters

[Rich Felker <dalias@libc.org>]

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