Re: [PATCH 2/3] y2038: linux: Provide __utimes64 implementation

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 20/02/2020 12:23, Lukasz Majewski wrote:
> Hi Adhemerval,
> 
>> On 07/02/2020 10:00, Lukasz Majewski wrote:
>>> This patch provides new __utimes64 explicit 64 bit function for
>>> setting file's 64 bit attributes for access and modification time.
>>>
>>> Internally, the __utimensat64_helper function is used. This patch
>>> is necessary for having architectures with __WORDSIZE == 32 Y2038
>>> safe.
>>>
>>> Moreover, a 32 bit version - __utimes has been refactored to
>>> internally use __utimes64.
>>>
>>> The __utimes is now supposed to be used on systems still supporting
>>> 32 bit time (__TIMESIZE != 64) - hence the necessary conversion of
>>> struct timeval to 64 bit struct __timeval64.
>>>
>>> Build tests:
>>> ./src/scripts/build-many-glibcs.py glibcs
>>>
>>> Run-time tests:
>>> - Run specific tests on ARM/x86 32bit systems (qemu):
>>>   https://github.com/lmajewski/meta-y2038 and run tests:
>>>   https://github.com/lmajewski/y2038-tests/commits/master
>>>
>>> Above tests were performed with Y2038 redirection applied as well
>>> as without to test proper usage of both __utimes64 and __utimes.  
>>
>> LGTM with some smalls changes below.
>>
>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
>>> ---
>>>  include/time.h                   |  3 +++
>>>  sysdeps/unix/sysv/linux/utimes.c | 37
>>> ++++++++++++++++++++++---------- 2 files changed, 29 insertions(+),
>>> 11 deletions(-)
>>>
>>> diff --git a/include/time.h b/include/time.h
>>> index e38f5e32e6..b04747889a 100644
>>> --- a/include/time.h
>>> +++ b/include/time.h
>>> @@ -211,8 +211,11 @@ libc_hidden_proto (__clock_getres64);
>>>  #endif
>>>  
>>>  #if __TIMESIZE == 64
>>> +# define __utimes64 __utimes
>>>  # define __utimensat64 __utimensat
>>>  #else
>>> +extern int __utimes64 (const char *file, const struct __timeval64
>>> tvp[2]); +libc_hidden_proto (__utimes64)
>>>  extern int __utimensat64 (int fd, const char *file,
>>>                            const struct __timespec64 tsp[2], int
>>> flags); libc_hidden_proto (__utimensat64);  
>>
>> Ok.
>>
>>> diff --git a/sysdeps/unix/sysv/linux/utimes.c
>>> b/sysdeps/unix/sysv/linux/utimes.c index 121d883469..09c4e56f18
>>> 100644 --- a/sysdeps/unix/sysv/linux/utimes.c
>>> +++ b/sysdeps/unix/sysv/linux/utimes.c
>>> @@ -16,22 +16,37 @@
>>>     License along with the GNU C Library; if not, see
>>>     <https://www.gnu.org/licenses/>.  */
>>>  
>>> -#include <errno.h>
>>> -#include <stddef.h>
>>> -#include <utime.h>
>>> -#include <sys/time.h>
>>> -#include <sysdep.h>
>>> +#include <time.h>
>>>  
>>> +int
>>> +__utimes64 (const char *file, const struct __timeval64 tvp[2])
>>> +{
>>> +  struct __timespec64 ts64[2];
>>> +
>>> +  if (tvp)  
>>
>> No implicit checks.
> 
> The documentation [1] and [2] explicitly says that times (here tvp) can
> be NULL:
> "If times is NULL, then the access and modification times of the file
> are set to the current time. "
> 
> Hence, it is perfectly valid to pass the NULL to the
> __utimensat64_helper(). Without this check we will segfault earlier
> (before we reach proper syscall) and introduce regression in glibc.

I meant to use:

  if (tvp != NULL)