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

[Samuel Holland <samuel@sholland.org>]

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