[1/2] y2038: linux: Provide __utimensat64 implementation
diff mbox series

Message ID 20191028165926.10351-1-lukma@denx.de
State New
Headers show
Series
  • [1/2] y2038: linux: Provide __utimensat64 implementation
Related show

Commit Message

Lukasz Majewski Oct. 28, 2019, 4:59 p.m. UTC
This patch provides new __utimensat64 explicit 64 bit function for
setting access and modification time of a file. Moreover, a 32 bit version -
__utimensat has been refactored to internally use __utimensat64.

The __utimensat is now supposed to be used on systems still supporting
32 bit time (__TIMESIZE != 64) - hence the necessary conversions to 64 bit
struct __timespec64.
When pointer to struct __timespec64 is NULL - the file access and modification
time is set to the current one and no conversions from struct timespec to
__timespec64 are performed.

The new utimensat_time64 syscall available from Linux 5.1+ has been used,
when applicable.
The new helper function - __utimensat64_helper - has been introduced to
facilitate code re-usage on function providing futimens syscall handling.
It also checks if passed nanoseconds are in the valid range as well as if one of
two special values - UTIME_NOW and UTIME_OMIT were not passed.
Moreover, when utimensat syscall on systems supporting 32 bit time ABI is used,
the check is performed if passed data fits into 32 bit time_t range.

Build tests:
- The code has been tested on x86_64/x86 (native compilation):
make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"

- The glibc has been build tested (make PARALLELMFLAGS="-j8") for
x86 (i386), x86_64-x32, and armv7

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

- Use of cross-test-ssh.sh for ARM (armv7):
make PARALLELMFLAGS="-j8" test-wrapper='./cross-test-ssh.sh root@192.168.7.2' xcheck

Linux kernel, headers and minimal kernel version for glibc build test
matrix:
- Linux v5.1 (with utimensat_time64) and glibc build with v5.1 as
minimal kernel version (--enable-kernel="5.1.0")
The __ASSUME_TIME64_SYSCALLS flag defined.

- Linux v5.1 and default minimal kernel version
The __ASSUME_TIME64_SYSCALLS not defined, but kernel supports utimensat_time64
syscall.

- Linux v4.19 (no utimensat_time64 support) with default minimal kernel
version for contemporary glibc
This kernel doesn't support utimensat_time64 syscall, so the fallback
to utimensat is tested.

The above tests were performed with Y2038 redirection applied as well as
without (so the __TIMESIZE != 64 execution path is checked as well).

No regressions were observed.
---
 include/time.h                      | 12 +++++
 sysdeps/unix/sysv/linux/utimensat.c | 76 +++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 4 deletions(-)

Comments

Joseph Myers Oct. 28, 2019, 6:11 p.m. UTC | #1
On Mon, 28 Oct 2019, Lukasz Majewski wrote:

> The new helper function - __utimensat64_helper - has been introduced to
> facilitate code re-usage on function providing futimens syscall handling.
> It also checks if passed nanoseconds are in the valid range as well as if one of
> two special values - UTIME_NOW and UTIME_OMIT were not passed.

In general we don't need such a check in userspace if the nanoseconds 
value will be passed to the kernel and it will do its own check.  I don't 
see such a check in the original code.  Is it being added to give the 
EINVAL error precedence over EOVERFLOW, in the case where a 64-bit time 
value with invalid nanoseconds is passed but a syscall using 32-bit time 
ends up being used?

> +  if (tsp64 && (! valid_nanoseconds(tsp64[0].tv_nsec)
> +                || ! valid_nanoseconds(tsp64[1].tv_nsec)))

Note missing space before '('.

> +    {
> +      if (tsp64[0].tv_nsec != UTIME_NOW && tsp64[0].tv_nsec != UTIME_OMIT
> +          && tsp64[1].tv_nsec != UTIME_NOW && tsp64[1].tv_nsec != UTIME_OMIT)
> +        {
> +          __set_errno (EINVAL);

I don't think this logic is correct; it would allow through cases where 
one timestamp is invalid and the other is UTIME_NOW or UTIME_OMIT, for 
example.

It might be better to have another helper function, e.g. 
valid_nanoseconds_for_utimensat, which calls valid_nanoseconds but also 
accepts the UTIME_* constants.
Lukasz Majewski Oct. 28, 2019, 11:15 p.m. UTC | #2
Hi Joseph,

> On Mon, 28 Oct 2019, Lukasz Majewski wrote:
> 
> > The new helper function - __utimensat64_helper - has been
> > introduced to facilitate code re-usage on function providing
> > futimens syscall handling. It also checks if passed nanoseconds are
> > in the valid range as well as if one of two special values -
> > UTIME_NOW and UTIME_OMIT were not passed.  
> 
> In general we don't need such a check in userspace if the nanoseconds 
> value will be passed to the kernel and it will do its own check.  I
> don't see such a check in the original code.

I must admit that I was a bit too cautious. If I remember correctly
kernel does the valid nano seconds check on its own, so it would be
correct to allow it to perform the check and return the error if wrong
range is detected.

>  Is it being added to
> give the EINVAL error precedence over EOVERFLOW, in the case where a
> 64-bit time value with invalid nanoseconds is passed but a syscall
> using 32-bit time ends up being used?

No. The nanoseconds check in the __utimensat_helper() function can be
removed completely.

Moreover, I do believe that the overflow check shall be kept to prevent
calling 32 bit utimensat syscall with passed 64 bit time value in
struct __timespec64.

> 
> > +  if (tsp64 && (! valid_nanoseconds(tsp64[0].tv_nsec)
> > +                || ! valid_nanoseconds(tsp64[1].tv_nsec)))  
> 
> Note missing space before '('.

Ok. I will fix it in v2.

> 
> > +    {
> > +      if (tsp64[0].tv_nsec != UTIME_NOW && tsp64[0].tv_nsec !=
> > UTIME_OMIT
> > +          && tsp64[1].tv_nsec != UTIME_NOW && tsp64[1].tv_nsec !=
> > UTIME_OMIT)
> > +        {
> > +          __set_errno (EINVAL);  
> 
> I don't think this logic is correct; it would allow through cases
> where one timestamp is invalid and the other is UTIME_NOW or
> UTIME_OMIT, for example.
> 
> It might be better to have another helper function, e.g. 
> valid_nanoseconds_for_utimensat, which calls valid_nanoseconds but
> also accepts the UTIME_* constants.
> 

As you stated above - this check is also performed in the Linux kernel,
so it can be safely removed from the __utimensat_helper function in
glibc.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Patch
diff mbox series

diff --git a/include/time.h b/include/time.h
index de660f7f57..432489ae29 100644
--- a/include/time.h
+++ b/include/time.h
@@ -141,6 +141,18 @@  extern int __clock_getres64 (clockid_t clock_id,
 libc_hidden_proto (__clock_getres64);
 #endif
 
+#if __TIMESIZE == 64
+# define __utimensat64 __utimensat
+#else
+extern int __utimensat64 (int fd, const char *file,
+                          const struct __timespec64 tsp[2], int flags);
+libc_hidden_proto (__utimensat64);
+#endif
+
+extern int __utimensat64_helper (int fd, const char *file,
+                                 const struct __timespec64 tsp[2], int flags);
+libc_hidden_proto (__utimensat64_helper);
+
 /* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
    and store year, yday, mon, mday, wday, hour, min, sec into *TP.
diff --git a/sysdeps/unix/sysv/linux/utimensat.c b/sysdeps/unix/sysv/linux/utimensat.c
index 3bffa7d22a..d4b9679d99 100644
--- a/sysdeps/unix/sysv/linux/utimensat.c
+++ b/sysdeps/unix/sysv/linux/utimensat.c
@@ -19,18 +19,86 @@ 
 #include <errno.h>
 #include <sys/stat.h>
 #include <sysdep.h>
+#include <time.h>
+#include <kernel-features.h>
 
+/* Helper function defined for easy reusage of the code which calls utimensat
+   and utimensat_time64 syscall.  */
+int
+__utimensat64_helper (int fd, const char *file,
+                      const struct __timespec64 tsp64[2], int flags)
+{
+  if (tsp64 && (! valid_nanoseconds(tsp64[0].tv_nsec)
+                || ! valid_nanoseconds(tsp64[1].tv_nsec)))
+    {
+      if (tsp64[0].tv_nsec != UTIME_NOW && tsp64[0].tv_nsec != UTIME_OMIT
+          && tsp64[1].tv_nsec != UTIME_NOW && tsp64[1].tv_nsec != UTIME_OMIT)
+        {
+          __set_errno (EINVAL);
+          return -1;
+        }
+    }
+
+#ifdef __ASSUME_TIME64_SYSCALLS
+# ifndef __NR_utimensat_time64
+#  define __NR_utimensat_time64 __NR_utimensat
+# endif
+  return INLINE_SYSCALL (utimensat_time64, 4, fd, file, &tsp64[0], flags);
+#else
+# ifdef __NR_utimensat_time64
+  int ret = INLINE_SYSCALL (utimensat_time64, 4, fd, file, &tsp64[0], flags);
+  if (ret == 0 || errno != ENOSYS)
+    return ret;
+# endif
+  if (tsp64
+      && (! in_time_t_range (tsp64[0].tv_sec)
+          || ! in_time_t_range (tsp64[1].tv_sec)))
+    {
+      __set_errno (EOVERFLOW);
+      return -1;
+    }
+
+  struct timespec tsp32[2];
+  if (tsp64)
+    {
+      tsp32[0] = valid_timespec64_to_timespec (tsp64[0]);
+      tsp32[1] = valid_timespec64_to_timespec (tsp64[1]);
+    }
+
+  return INLINE_SYSCALL (utimensat, 4, fd, file, tsp64 ? &tsp32[0] : NULL,
+                         flags);
+#endif
+
+}
+libc_hidden_def (__utimensat64_helper)
 
 /* Change the access time of FILE to TSP[0] and
    the modification time of FILE to TSP[1].
 
    Starting with 2.6.22 the Linux kernel has the utimensat syscall.  */
 int
-utimensat (int fd, const char *file, const struct timespec tsp[2],
-	   int flags)
+__utimensat64 (int fd, const char *file, const struct __timespec64 tsp64[2],
+               int flags)
 {
   if (file == NULL)
     return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-  /* Avoid implicit array coercion in syscall macros.  */
-  return INLINE_SYSCALL (utimensat, 4, fd, file, &tsp[0], flags);
+
+  return __utimensat64_helper (fd, file, &tsp64[0], flags);
+}
+
+#if __TIMESIZE != 64
+int
+__utimensat (int fd, const char *file, const struct timespec tsp[2],
+             int flags)
+{
+  struct __timespec64 tsp64[2];
+  if (tsp)
+    {
+      tsp64[0] = valid_timespec_to_timespec64 (tsp[0]);
+      tsp64[1] = valid_timespec_to_timespec64 (tsp[1]);
+    }
+
+  return __utimensat64 (fd, file, tsp ? &tsp64[0] : NULL, flags);
 }
+#endif
+weak_alias (__utimensat, utimensat)