From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vs1-f51.google.com (mail-vs1-f51.google.com [209.85.217.51]) by sourceware.org (Postfix) with ESMTPS id 3B7D93858D20 for ; Tue, 8 Nov 2022 02:39:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3B7D93858D20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=debian.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-vs1-f51.google.com with SMTP id p4so12404879vsa.11 for ; Mon, 07 Nov 2022 18:39:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=klN4kJXEA/KYsbRDqx5j1XJOs5N4+Lv/bi0JKN5ueWk=; b=1xN0hAPYR3fIsYyfwfKm5ovifIh7rDqCFnG+/RQms6PpN0eQE1CBGxS50LMCYeIfvp k6Qx270ZeviiC6EWgmhicwUAFJuySEvu+3Epvu2TGtHLQHpW5g2kNPi8ka0DI4SF6GFP 5d83yryFch2ht3Icn7ifOwZ02K20bf+GvUcoe20hjvITW7YRT+E2J/fL54VfbFeTV1+S HBahcBH9eklFVz29pEP1wDCAE6qqxzvZIirKOozuPQLMZaU7bzwL8fQewqOxYx2jlUNl rVysNSjFyyg3E3+MR5sYil4tirKPu01BscogpHe6bQfof7GlaoQnv7alo+WML27/3cqD 6e0Q== X-Gm-Message-State: ACrzQf0vLADtMWqB6cM8HpjeHBu9yCqo+o2v8oTcPJGEOPbkAP+5p15z K3rbPO+BcWmPge0zp6BnRLfnMtRpA4OWo1/rfcI= X-Google-Smtp-Source: AMsMyM56Zp78xjwPHoreaG9CJ/bQZ6YtPf597tgWLnlciL5XtU5REkbkBENP4yi6rmw4LM680Ti245hA/EjUQyFtl7A= X-Received: by 2002:a67:f558:0:b0:3ad:4c6b:dcd7 with SMTP id z24-20020a67f558000000b003ad4c6bdcd7mr16787882vsn.24.1667875152570; Mon, 07 Nov 2022 18:39:12 -0800 (PST) MIME-Version: 1.0 References: <20221104014757.1544793-1-yunqiang.su@cipunited.com> <52e1dbe6-3af2-fba3-38ef-5c05d2d4f2d6@linaro.org> In-Reply-To: From: YunQiang Su Date: Tue, 8 Nov 2022 10:39:00 +0800 Message-ID: Subject: Re: [PATCH v2] Use in_int32_t_range to check need 64bit syscall To: Adhemerval Zanella Netto Cc: YunQiang Su , libc-alpha@sourceware.org, aurelien@aurel32.net, jiaxun.yang@flygoat.com, macro@orcam.me.uk Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00,BODY_8BITS,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: YunQiang Su =E4=BA=8E2022=E5=B9=B411=E6=9C=888=E6=97=A5=E5= =91=A8=E4=BA=8C 10:26=E5=86=99=E9=81=93=EF=BC=9A > > Adhemerval Zanella Netto =E4=BA=8E2022=E5= =B9=B411=E6=9C=888=E6=97=A5=E5=91=A8=E4=BA=8C 03:49=E5=86=99=E9=81=93=EF=BC= =9A > > > > > > > > On 03/11/22 22:47, YunQiang Su wrote: > > > The current version, we use in_time_t_range to detect overflow > > > of a time. Whenever overflow we try to use 64bit version syscall. > > > > > > While the function name is quite confused. So let's use a new > > > function called in_int32_t_range to do so. > > > > > > We still keep in_time_t_range, and use it to detect overflow of > > > the return value from syscalls. > > > > The commit message sounds confusing, maybe: > > > > "Currently glibc uses in_time_t_range to detects time_t overflow, > > and if it occurs fallbacks to 64 bit syscall version. > > > > The function name is confusing because internally time_t might be > > either 32 bits or 64 bits (depending on __TIMESIZE). > > > > This patch refactors the in_time_t_range by replacing it with > > in_int32_t_range for the case to check if the 64 bit time_t syscall > > should be used. > > > > The in_time_t range is used to detect overflow of the > > syscall return value". > > > > Yes. Your description is much better. > > > > --- > > > include/time.h | 10 +++++++++- > > > nptl/futex-internal.c | 4 ++-- > > > sysdeps/unix/sysv/linux/clock_adjtime.c | 2 +- > > > sysdeps/unix/sysv/linux/clock_nanosleep.c | 2 +- > > > sysdeps/unix/sysv/linux/clock_settime.c | 2 +- > > > sysdeps/unix/sysv/linux/mq_timedreceive.c | 2 +- > > > sysdeps/unix/sysv/linux/mq_timedsend.c | 2 +- > > > sysdeps/unix/sysv/linux/ppoll.c | 2 +- > > > sysdeps/unix/sysv/linux/pselect.c | 2 +- > > > sysdeps/unix/sysv/linux/recvmmsg.c | 2 +- > > > sysdeps/unix/sysv/linux/select.c | 2 +- > > > sysdeps/unix/sysv/linux/semtimedop.c | 2 +- > > > sysdeps/unix/sysv/linux/setitimer.c | 4 ++-- > > > sysdeps/unix/sysv/linux/setsockopt.c | 2 +- > > > sysdeps/unix/sysv/linux/sigtimedwait.c | 2 +- > > > sysdeps/unix/sysv/linux/timer_settime.c | 4 ++-- > > > sysdeps/unix/sysv/linux/timerfd_settime.c | 4 ++-- > > > sysdeps/unix/sysv/linux/utimensat.c | 4 ++-- > > > 18 files changed, 31 insertions(+), 23 deletions(-) > > > > > > diff --git a/include/time.h b/include/time.h > > > index 20abea69d4..f599eeed4e 100644 > > > --- a/include/time.h > > > +++ b/include/time.h > > > @@ -347,12 +347,20 @@ libc_hidden_proto (__time64) > > > /* Check whether T fits in int32_t, assume all usages are for > > > sizeof(time_t) =3D=3D 32. */ > > > static inline bool > > > -in_time_t_range (__time64_t t) > > > +in_int32_t_range (__time64_t t) > > > { > > > int32_t s =3D t; > > > return s =3D=3D t; > > > } > > > > > > +/* Check whether T fits in time_t. */ > > > +static inline bool > > > +in_time_t_range (__time64_t t) > > > +{ > > > + time_t s =3D t; > > > + return s =3D=3D t; > > > +} > > > + > > > > I think it might be better to avoid define this function for __TIMESIZE= =3D=3D 64, > > the function always returns true and for 64 bit time_t its occurence mi= ght > > be a misuse. > > > > The reason it exists on 64bit platforms is due to the fact that we may > need to share > code on both 32bit and 64bit platforms. > > So I think that we should keep it. > > For performance, I think that we can make in_time_t_range return true dir= ectly. > Ohh, the compilers are smart enough to take care of it. So we don't need it. > > The rest of the path looks ok, the remaining in_time_t_range usage are = only > > for check of syscall return value (besides the generic usage on mktime = and > > gmtime). > > > > > /* Convert a known valid struct timeval into a struct __timespec64. = */ > > > static inline struct __timespec64 > > > valid_timeval_to_timespec64 (const struct timeval tv) > > > diff --git a/nptl/futex-internal.c b/nptl/futex-internal.c > > > index 7ec228e8fb..6b8ac2304d 100644 > > > --- a/nptl/futex-internal.c > > > +++ b/nptl/futex-internal.c > > > @@ -87,7 +87,7 @@ __futex_abstimed_wait_common (unsigned int* futex_w= ord, > > > err =3D __futex_abstimed_wait_common64 (futex_word, expected, op, = abstime, > > > private, cancel); > > > #else > > > - bool need_time64 =3D abstime !=3D NULL && !in_time_t_range (abstim= e->tv_sec); > > > + bool need_time64 =3D abstime !=3D NULL && !in_int32_t_range (absti= me->tv_sec); > > > if (need_time64) > > > { > > > err =3D __futex_abstimed_wait_common64 (futex_word, expected, = op, abstime, > > > @@ -162,7 +162,7 @@ __futex_lock_pi64 (int *futex_word, clockid_t clo= ckid, > > > # ifdef __ASSUME_TIME64_SYSCALLS > > > err =3D INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, 0,= abstime); > > > # else > > > - bool need_time64 =3D abstime !=3D NULL && !in_time_t_range (abstim= e->tv_sec); > > > + bool need_time64 =3D abstime !=3D NULL && !in_int32_t_range (absti= me->tv_sec); > > > if (need_time64) > > > err =3D INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, = 0, abstime); > > > else > > > diff --git a/sysdeps/unix/sysv/linux/clock_adjtime.c b/sysdeps/unix/s= ysv/linux/clock_adjtime.c > > > index 5ded82f506..63ea68c3af 100644 > > > --- a/sysdeps/unix/sysv/linux/clock_adjtime.c > > > +++ b/sysdeps/unix/sysv/linux/clock_adjtime.c > > > @@ -35,7 +35,7 @@ __clock_adjtime64 (const clockid_t clock_id, struct= __timex64 *tx64) > > > return r; > > > > > > if (tx64->modes & ADJ_SETOFFSET > > > - && ! in_time_t_range (tx64->time.tv_sec)) > > > + && ! in_int32_t_range (tx64->time.tv_sec)) > > > { > > > __set_errno (EOVERFLOW); > > > return -1; > > > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix= /sysv/linux/clock_nanosleep.c > > > index e610fd4e8d..9031060486 100644 > > > --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c > > > +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c > > > @@ -48,7 +48,7 @@ __clock_nanosleep_time64 (clockid_t clock_id, int f= lags, > > > r =3D INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, clock_id, f= lags, req, > > > rem); > > > #else > > > - if (!in_time_t_range (req->tv_sec)) > > > + if (!in_int32_t_range (req->tv_sec)) > > > { > > > r =3D INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, clock_i= d, flags, > > > req, rem); > > > diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/s= ysv/linux/clock_settime.c > > > index 2a32e2eb13..db9698ba0d 100644 > > > --- a/sysdeps/unix/sysv/linux/clock_settime.c > > > +++ b/sysdeps/unix/sysv/linux/clock_settime.c > > > @@ -41,7 +41,7 @@ __clock_settime64 (clockid_t clock_id, const struct= __timespec64 *tp) > > > if (ret =3D=3D 0 || errno !=3D ENOSYS) > > > return ret; > > > > > > - if (! in_time_t_range (tp->tv_sec)) > > > + if (! in_int32_t_range (tp->tv_sec)) > > > { > > > __set_errno (EOVERFLOW); > > > return -1; > > > diff --git a/sysdeps/unix/sysv/linux/mq_timedreceive.c b/sysdeps/unix= /sysv/linux/mq_timedreceive.c > > > index 5bf1e0a83b..6c719d4b70 100644 > > > --- a/sysdeps/unix/sysv/linux/mq_timedreceive.c > > > +++ b/sysdeps/unix/sysv/linux/mq_timedreceive.c > > > @@ -36,7 +36,7 @@ ___mq_timedreceive_time64 (mqd_t mqdes, char *__res= trict msg_ptr, size_t msg_len > > > msg_prio, abs_timeout); > > > #else > > > bool need_time64 =3D abs_timeout !=3D NULL > > > - && !in_time_t_range (abs_timeout->tv_sec); > > > + && !in_int32_t_range (abs_timeout->tv_sec); > > > if (need_time64) > > > { > > > int r =3D SYSCALL_CANCEL (mq_timedreceive_time64, mqdes, msg_p= tr, msg_len, > > > diff --git a/sysdeps/unix/sysv/linux/mq_timedsend.c b/sysdeps/unix/sy= sv/linux/mq_timedsend.c > > > index 3ca5050976..eb8353feef 100644 > > > --- a/sysdeps/unix/sysv/linux/mq_timedsend.c > > > +++ b/sysdeps/unix/sysv/linux/mq_timedsend.c > > > @@ -36,7 +36,7 @@ ___mq_timedsend_time64 (mqd_t mqdes, const char *ms= g_ptr, size_t msg_len, > > > msg_prio, abs_timeout); > > > #else > > > bool need_time64 =3D abs_timeout !=3D NULL > > > - && !in_time_t_range (abs_timeout->tv_sec); > > > + && !in_int32_t_range (abs_timeout->tv_sec); > > > if (need_time64) > > > { > > > int r =3D SYSCALL_CANCEL (mq_timedsend_time64, mqdes, msg_ptr,= msg_len, > > > diff --git a/sysdeps/unix/sysv/linux/ppoll.c b/sysdeps/unix/sysv/linu= x/ppoll.c > > > index 1105e29b00..6653e1bc55 100644 > > > --- a/sysdeps/unix/sysv/linux/ppoll.c > > > +++ b/sysdeps/unix/sysv/linux/ppoll.c > > > @@ -43,7 +43,7 @@ __ppoll64 (struct pollfd *fds, nfds_t nfds, const s= truct __timespec64 *timeout, > > > __NSIG_BYTES); > > > #else > > > int ret; > > > - bool need_time64 =3D timeout !=3D NULL && !in_time_t_range (timeou= t->tv_sec); > > > + bool need_time64 =3D timeout !=3D NULL && !in_int32_t_range (timeo= ut->tv_sec); > > > if (need_time64) > > > { > > > ret =3D SYSCALL_CANCEL (ppoll_time64, fds, nfds, timeout, sigm= ask, > > > diff --git a/sysdeps/unix/sysv/linux/pselect.c b/sysdeps/unix/sysv/li= nux/pselect.c > > > index eba1c111f8..23fd0dfad6 100644 > > > --- a/sysdeps/unix/sysv/linux/pselect.c > > > +++ b/sysdeps/unix/sysv/linux/pselect.c > > > @@ -56,7 +56,7 @@ __pselect64 (int nfds, fd_set *readfds, fd_set *wri= tefds, fd_set *exceptfds, > > > return pselect64_syscall (nfds, readfds, writefds, exceptfds, time= out, > > > sigmask); > > > #else > > > - bool need_time64 =3D timeout !=3D NULL && !in_time_t_range (timeou= t->tv_sec); > > > + bool need_time64 =3D timeout !=3D NULL && !in_int32_t_range (timeo= ut->tv_sec); > > > if (need_time64) > > > { > > > int r =3D pselect64_syscall (nfds, readfds, writefds, exceptfd= s, timeout, > > > diff --git a/sysdeps/unix/sysv/linux/recvmmsg.c b/sysdeps/unix/sysv/l= inux/recvmmsg.c > > > index d9664d61c4..1ba5a4f25c 100644 > > > --- a/sysdeps/unix/sysv/linux/recvmmsg.c > > > +++ b/sysdeps/unix/sysv/linux/recvmmsg.c > > > @@ -35,7 +35,7 @@ recvmmsg_syscall (int fd, struct mmsghdr *vmessages= , unsigned int vlen, > > > struct timespec ts32, *pts32 =3D NULL; > > > if (timeout !=3D NULL) > > > { > > > - if (! in_time_t_range (timeout->tv_sec)) > > > + if (! in_int32_t_range (timeout->tv_sec)) > > > { > > > __set_errno (EINVAL); > > > return -1; > > > diff --git a/sysdeps/unix/sysv/linux/select.c b/sysdeps/unix/sysv/lin= ux/select.c > > > index a3f0a2eba7..fd147dfdd4 100644 > > > --- a/sysdeps/unix/sysv/linux/select.c > > > +++ b/sysdeps/unix/sysv/linux/select.c > > > @@ -72,7 +72,7 @@ __select64 (int nfds, fd_set *readfds, fd_set *writ= efds, fd_set *exceptfds, > > > TIMESPEC_TO_TIMEVAL (timeout, pts64); > > > return r; > > > #else > > > - bool need_time64 =3D timeout !=3D NULL && !in_time_t_range (timeou= t->tv_sec); > > > + bool need_time64 =3D timeout !=3D NULL && !in_int32_t_range (timeo= ut->tv_sec); > > > if (need_time64) > > > { > > > int r =3D SYSCALL_CANCEL (pselect6_time64, nfds, readfds, writ= efds, > > > diff --git a/sysdeps/unix/sysv/linux/semtimedop.c b/sysdeps/unix/sysv= /linux/semtimedop.c > > > index 38a401bb6f..0cdc0fad4b 100644 > > > --- a/sysdeps/unix/sysv/linux/semtimedop.c > > > +++ b/sysdeps/unix/sysv/linux/semtimedop.c > > > @@ -42,7 +42,7 @@ __semtimedop64 (int semid, struct sembuf *sops, siz= e_t nsops, > > > #ifdef __ASSUME_TIME64_SYSCALLS > > > return semtimedop_syscall (semid, sops, nsops, timeout); > > > #else > > > - bool need_time64 =3D timeout !=3D NULL && !in_time_t_range (timeou= t->tv_sec); > > > + bool need_time64 =3D timeout !=3D NULL && !in_int32_t_range (timeo= ut->tv_sec); > > > if (need_time64) > > > { > > > int r =3D semtimedop_syscall (semid, sops, nsops, timeout); > > > diff --git a/sysdeps/unix/sysv/linux/setitimer.c b/sysdeps/unix/sysv/= linux/setitimer.c > > > index 6d5183aa63..55e6003e0f 100644 > > > --- a/sysdeps/unix/sysv/linux/setitimer.c > > > +++ b/sysdeps/unix/sysv/linux/setitimer.c > > > @@ -32,8 +32,8 @@ __setitimer64 (__itimer_which_t which, > > > #else > > > struct __itimerval32 new_value_32; > > > > > > - if (! in_time_t_range (new_value->it_interval.tv_sec) > > > - || ! in_time_t_range (new_value->it_value.tv_sec)) > > > + if (! in_int32_t_range (new_value->it_interval.tv_sec) > > > + || ! in_int32_t_range (new_value->it_value.tv_sec)) > > > { > > > __set_errno (EOVERFLOW); > > > return -1; > > > diff --git a/sysdeps/unix/sysv/linux/setsockopt.c b/sysdeps/unix/sysv= /linux/setsockopt.c > > > index 4292f411f3..6506ca03ab 100644 > > > --- a/sysdeps/unix/sysv/linux/setsockopt.c > > > +++ b/sysdeps/unix/sysv/linux/setsockopt.c > > > @@ -54,7 +54,7 @@ setsockopt32 (int fd, int level, int optname, const= void *optval, > > > } > > > > > > struct __timeval64 *tv64 =3D (struct __timeval64 *) optval; > > > - if (! in_time_t_range (tv64->tv_sec)) > > > + if (! in_int32_t_range (tv64->tv_sec)) > > > { > > > __set_errno (EOVERFLOW); > > > break; > > > diff --git a/sysdeps/unix/sysv/linux/sigtimedwait.c b/sysdeps/unix/sy= sv/linux/sigtimedwait.c > > > index 069fa1d5e4..1993e3f85a 100644 > > > --- a/sysdeps/unix/sysv/linux/sigtimedwait.c > > > +++ b/sysdeps/unix/sysv/linux/sigtimedwait.c > > > @@ -31,7 +31,7 @@ __sigtimedwait64 (const sigset_t *set, siginfo_t *i= nfo, > > > result =3D SYSCALL_CANCEL (rt_sigtimedwait_time64, set, info, time= out, > > > __NSIG_BYTES); > > > #else > > > - bool need_time64 =3D timeout !=3D NULL && !in_time_t_range (timeou= t->tv_sec); > > > + bool need_time64 =3D timeout !=3D NULL && !in_int32_t_range (timeo= ut->tv_sec); > > > if (need_time64) > > > { > > > result =3D SYSCALL_CANCEL (rt_sigtimedwait_time64, set, info, = timeout, > > > diff --git a/sysdeps/unix/sysv/linux/timer_settime.c b/sysdeps/unix/s= ysv/linux/timer_settime.c > > > index 518fed9c59..056e0aff59 100644 > > > --- a/sysdeps/unix/sysv/linux/timer_settime.c > > > +++ b/sysdeps/unix/sysv/linux/timer_settime.c > > > @@ -46,8 +46,8 @@ ___timer_settime64 (timer_t timerid, int flags, > > > # endif > > > struct itimerspec its32, oits32; > > > > > > - if (! in_time_t_range ((value->it_value).tv_sec) > > > - || ! in_time_t_range ((value->it_interval).tv_sec)) > > > + if (! in_int32_t_range ((value->it_value).tv_sec) > > > + || ! in_int32_t_range ((value->it_interval).tv_sec)) > > > { > > > __set_errno (EOVERFLOW); > > > return -1; > > > diff --git a/sysdeps/unix/sysv/linux/timerfd_settime.c b/sysdeps/unix= /sysv/linux/timerfd_settime.c > > > index 59282bf1ad..c5f3f14c3b 100644 > > > --- a/sysdeps/unix/sysv/linux/timerfd_settime.c > > > +++ b/sysdeps/unix/sysv/linux/timerfd_settime.c > > > @@ -33,8 +33,8 @@ __timerfd_settime64 (int fd, int flags, const struc= t __itimerspec64 *value, > > > #ifdef __ASSUME_TIME64_SYSCALLS > > > return INLINE_SYSCALL_CALL (timerfd_settime64, fd, flags, value, o= value); > > > #else > > > - bool need_time64 =3D !in_time_t_range (value->it_value.tv_sec) > > > - || !in_time_t_range (value->it_interval.tv_sec); > > > + bool need_time64 =3D !in_int32_t_range (value->it_value.tv_sec) > > > + || !in_int32_t_range (value->it_interval.tv_sec); > > > if (need_time64) > > > { > > > int r =3D INLINE_SYSCALL_CALL (timerfd_settime64, fd, flags, v= alue, > > > diff --git a/sysdeps/unix/sysv/linux/utimensat.c b/sysdeps/unix/sysv/= linux/utimensat.c > > > index 5662ecc9aa..54a5e47618 100644 > > > --- a/sysdeps/unix/sysv/linux/utimensat.c > > > +++ b/sysdeps/unix/sysv/linux/utimensat.c > > > @@ -41,9 +41,9 @@ __utimensat64_helper (int fd, const char *file, > > > > > > bool need_time64 =3D tsp64 !=3D NULL > > > && ((!TS_SPECIAL (tsp64[0]) > > > - && !in_time_t_range (tsp64[0].tv_sec)) > > > + && !in_int32_t_range (tsp64[0].tv_sec)) > > > || (!TS_SPECIAL (tsp64[1]) > > > - && !in_time_t_range (tsp64[1].tv_sec))); > > > + && !in_int32_t_range (tsp64[1].tv_sec))); > > > if (need_time64) > > > { > > > int r =3D INLINE_SYSCALL_CALL (utimensat_time64, fd, file, &ts= p64[0],