Message ID | 20191028165926.10351-1-lukma@denx.de |
---|---|
State | New |
Headers | show |
Series | [1/2] y2038: linux: Provide __utimensat64 implementation | expand |
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.
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
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)