Message ID | 20200207130009.19396-3-lukma@denx.de |
---|---|
State | Committed |
Headers |
Received: (qmail 118571 invoked by alias); 7 Feb 2020 13:01:07 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 118388 invoked by uid 89); 7 Feb 2020 13:01:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 spammy= X-HELO: mail-out.m-online.net From: Lukasz Majewski <lukma@denx.de> To: Joseph Myers <joseph@codesourcery.com>, Paul Eggert <eggert@cs.ucla.edu>, Adhemerval Zanella <adhemerval.zanella@linaro.org> Cc: Alistair Francis <alistair23@gmail.com>, Alistair Francis <alistair.francis@wdc.com>, GNU C Library <libc-alpha@sourceware.org>, Siddhesh Poyarekar <siddhesh@gotplt.org>, Florian Weimer <fweimer@redhat.com>, Florian Weimer <fw@deneb.enyo.de>, Zack Weinberg <zackw@panix.com>, Carlos O'Donell <carlos@redhat.com>, Andreas Schwab <schwab@suse.de>, Lukasz Majewski <lukma@denx.de> Subject: [PATCH 2/3] y2038: linux: Provide __utimes64 implementation Date: Fri, 7 Feb 2020 14:00:08 +0100 Message-Id: <20200207130009.19396-3-lukma@denx.de> In-Reply-To: <20200207130009.19396-1-lukma@denx.de> References: <20200207130009.19396-1-lukma@denx.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Lukasz Majewski
Feb. 7, 2020, 1 p.m. UTC
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. --- include/time.h | 3 +++ sysdeps/unix/sysv/linux/utimes.c | 37 ++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 11 deletions(-)
Comments
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. > + { > + ts64[0] = timeval64_to_timespec64 (tvp[0]); > + ts64[1] = timeval64_to_timespec64 (tvp[1]); > + } > + > + return __utimensat64_helper (0, file, tvp ? ts64 : NULL, 0); > +} Ok. > > -/* Consider moving to syscalls.list. */ > +#if __TIMESIZE != 64 > +libc_hidden_def (__utimes64) > > -/* Change the access time of FILE to TVP[0] and > - the modification time of FILE to TVP[1]. */ > int > __utimes (const char *file, const struct timeval tvp[2]) > { > - /* Avoid implicit array coercion in syscall macros. */ > - return INLINE_SYSCALL (utimes, 2, file, &tvp[0]); > -} > + struct __timeval64 tv64[2]; > > + if (tvp) No implicit checks. > + { > + tv64[0] = valid_timeval_to_timeval64 (tvp[0]); > + tv64[1] = valid_timeval_to_timeval64 (tvp[1]); > + } > + > + return __utimes64 (file, tvp ? tv64 : NULL); > +} > +#endif > weak_alias (__utimes, utimes) > Ok.
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. Links: [1] - https://linux.die.net/man/2/utimes [2] - https://www.unix.com/man-page/linux/2/utimes/ > > > + { > > + ts64[0] = timeval64_to_timespec64 (tvp[0]); > > + ts64[1] = timeval64_to_timespec64 (tvp[1]); > > + } > > + > > + return __utimensat64_helper (0, file, tvp ? ts64 : NULL, 0); > > +} > > Ok. > > > > > -/* Consider moving to syscalls.list. */ > > +#if __TIMESIZE != 64 > > +libc_hidden_def (__utimes64) > > > > -/* Change the access time of FILE to TVP[0] and > > - the modification time of FILE to TVP[1]. */ > > int > > __utimes (const char *file, const struct timeval tvp[2]) > > { > > - /* Avoid implicit array coercion in syscall macros. */ > > - return INLINE_SYSCALL (utimes, 2, file, &tvp[0]); > > -} > > + struct __timeval64 tv64[2]; > > > > + if (tvp) > > No implicit checks. Please consider the above comment. > > > + { > > + tv64[0] = valid_timeval_to_timeval64 (tvp[0]); > > + tv64[1] = valid_timeval_to_timeval64 (tvp[1]); > > + } > > + > > + return __utimes64 (file, tvp ? tv64 : NULL); > > +} > > +#endif > > weak_alias (__utimes, utimes) > > > > Ok. 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
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)
Hi Adhemerval, > 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) Ach ... :-) Thanks for the clarification. I thought that this check is not needed at all. My misunderstanding :-) 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 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); 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) + { + ts64[0] = timeval64_to_timespec64 (tvp[0]); + ts64[1] = timeval64_to_timespec64 (tvp[1]); + } + + return __utimensat64_helper (0, file, tvp ? ts64 : NULL, 0); +} -/* Consider moving to syscalls.list. */ +#if __TIMESIZE != 64 +libc_hidden_def (__utimes64) -/* Change the access time of FILE to TVP[0] and - the modification time of FILE to TVP[1]. */ int __utimes (const char *file, const struct timeval tvp[2]) { - /* Avoid implicit array coercion in syscall macros. */ - return INLINE_SYSCALL (utimes, 2, file, &tvp[0]); -} + struct __timeval64 tv64[2]; + if (tvp) + { + tv64[0] = valid_timeval_to_timeval64 (tvp[0]); + tv64[1] = valid_timeval_to_timeval64 (tvp[1]); + } + + return __utimes64 (file, tvp ? tv64 : NULL); +} +#endif weak_alias (__utimes, utimes)