From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 60D9C383D009 for ; Sat, 12 Jun 2021 09:19:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 60D9C383D009 Received: by mail-pg1-x52f.google.com with SMTP id i34so4452039pgl.9 for ; Sat, 12 Jun 2021 02:19:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=8r7cRJxLc9tXKfZ1MOepAfrfWpueAJ+dJYrE8YLg1mU=; b=ogdepweJ4j9tGGWumWrIYp+ZUnvlYm1lPZ2iRUW0zLXTKsKwP2nE/vk/PzriZrXts1 CiM++2H4KQ8xTT0pP0gLyZlmT76QpswfX1DssV+x8l8DbymF6HR+p2F5z2TJ5zMoYQNM KpVhTzuZdkQOmOTCJrpK2+19U/EGufelxldS9zi3H0Jp13bYcqs/kUuOe8hmPWBTmRag /e0ju05Wtqvl+QCjdSbMznahpVw9z8AMUE6Zo1NyG+goZriYZmpIlhBM5tfYs2jbvFMX NzjD0jE/2PmLyuzrSL0VkiDUeJikZLzmBJDmj7IRFrxhovCnmreOfE98Cq6CM4RIfJXY Ftgg== X-Gm-Message-State: AOAM531BuwByRp+SmTXKIlmrL4LITvPtX2a+NOk5kIjo9wZUCtvgDtey SNaXrC+iaI3BaTqq83X05EQ= X-Google-Smtp-Source: ABdhPJxCuq8ei3iePeqvg3BfznmjKS3UXCf9pmCe2THgs88GdkK2tp1cGyFiEti6BfGz1lTH0MXqtA== X-Received: by 2002:a63:ef4c:: with SMTP id c12mr7814668pgk.441.1623489575557; Sat, 12 Jun 2021 02:19:35 -0700 (PDT) Received: from localhost (g151.115-65-219.ppp.wakwak.ne.jp. [115.65.219.151]) by smtp.gmail.com with ESMTPSA id e2sm8467889pjc.37.2021.06.12.02.19.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 12 Jun 2021 02:19:34 -0700 (PDT) Date: Sat, 12 Jun 2021 18:19:32 +0900 From: Stafford Horne To: Adhemerval Zanella Cc: GLIBC patches , Alistair Francis Subject: Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems Message-ID: References: <20210607131851.4015661-1-shorne@gmail.com> <8ad55354-5d41-eaa4-6d6c-e650ea30d7cc@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 12 Jun 2021 09:19:38 -0000 On Thu, Jun 10, 2021 at 06:38:04AM +0900, Stafford Horne wrote: > On Wed, Jun 09, 2021 at 10:50:23AM -0300, Adhemerval Zanella wrote: > > > > > > On 07/06/2021 10:18, Stafford Horne wrote: > > > On the port of OpenRISC I am working on and it appears the rv32 port > > > we have sets __TIMESIZE == 64 && __WORDSIZE == 32. This causes the > > > size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit > > > causing truncation. > > > > > > The truncations are unavoidable on these systems so skip the > > > testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64. > > > > Sigh, I was hoping that we won't need to handle this situation (glibc > > support only 64-bit time_t, but kernel still providing some 32-bit > > syscall). > > > > > --- > > > > > > I am open to other suggestions, this seemed the most correct to me. > > > > > > Cc: Adhemerval Zanella > > > Cc: Alistair Francis > > > > > > time/tst-itimer.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c > > > index 929c2b74c7..0c99d46d7e 100644 > > > --- a/time/tst-itimer.c > > > +++ b/time/tst-itimer.c > > > @@ -89,6 +89,7 @@ do_test (void) > > > TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec); > > > TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec); > > > > > > +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 > > > if (sizeof (time_t) == 4) > > > continue; > > > > > > @@ -146,6 +147,7 @@ do_test (void) > > > TEST_COMPARE (setitimer (timers[i], &it, NULL), -1); > > > TEST_COMPARE (errno, EOVERFLOW); > > > } > > > +#endif > > > } > > > > > > { > > > > > > > Instead of disabling, I think it would be better to use > > __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof > > (so we can still tests the EOVERFLOW): > > > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c > > index 929c2b74c7..bd7d7afe83 100644 > > --- a/time/tst-itimer.c > > +++ b/time/tst-itimer.c > > @@ -100,7 +100,7 @@ do_test (void) > > > > /* Linux does not provide 64 bit time_t support for getitimer and > > setitimer on architectures with 32 bit time_t support. */ > > - if (sizeof (__time_t) == 8) > > + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) > > { > > TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); > > TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 }, > > @@ -131,7 +131,7 @@ do_test (void) > > it.it_interval.tv_usec = 20; > > it.it_value.tv_sec = 30; > > it.it_value.tv_usec = 40; > > - if (sizeof (__time_t) == 8) > > + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) > > { > > TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); > > This looks good to me, I can update to this, test and resend the patch when I > get some time. Probably later tonight. I tested this and it exposes an issue in the linux setitimer wrapper. On my platform I get EINVAL instead of EOVERFLOW. FAIL: time/tst-itimer original exit status 1 tst-itimer.c:125: numeric comparison failure left: 22 (0x16); from: errno right: 75 (0x4b); from: EOVERFLOW tst-itimer.c:147: numeric comparison failure left: 22 (0x16); from: errno right: 75 (0x4b); from: EOVERFLOW It seems this is because sysdeps/unix/sysv/linux/setitimer.c, checks that the incoming value is in the range of time_t. The problem is that that we need to fit the value in __int32_t not time_t. When testing the time_t range check does not detect the overflow and setitimer ends up passing a -1 to the kernel causing EINVAL. I can fix that, as per the patch below, but It will take me some time to audit other places this might be an issue. if (! in_time_t_range (new_value->it_interval.tv_sec) || ! in_time_t_range (new_value->it_value.tv_sec)) { __set_errno (EOVERFLOW); return -1; } new_value_32.it_interval = valid_timeval64_to_timeval32 (new_value->it_interval); new_value_32.it_value = valid_timeval64_to_timeval32 (new_value->it_value); The below patch works for me, but there is probably a better thing to do then create a new functrion. diff --git a/include/time.h b/include/time.h index 4372bfbd96..377a4a45ea 100644 --- a/include/time.h +++ b/include/time.h @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t) return s == t; } +/* Check whether T fits in a timeval32 (__int32_t). */ +static inline bool +in_timeval32_range (__time64_t t) +{ + __int32_t s = t; + return s == t; +} + /* 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/sysdeps/unix/sysv/linux/setitimer.c b/sysdeps/unix/sysv/linux/setitimer.c index 083a25cf35..bada30ba02 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_timeval32_range (new_value->it_interval.tv_sec) + || ! in_timeval32_range (new_value->it_value.tv_sec)) { -Stafford