From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) by sourceware.org (Postfix) with ESMTPS id CAA2C383E83D for ; Tue, 6 Jul 2021 19:59:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CAA2C383E83D Received: by mail-pf1-x436.google.com with SMTP id f20so65337pfa.1 for ; Tue, 06 Jul 2021 12:59:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=dZneDkVDeQOvNEKxvTIHpO7W8ciy+b6MsvWI0Df3qSA=; b=l5szSa1EE757aoOtJWohG2bNByHhESNpMTyFN7YDCMoD3I02qc/VhzpBmlrg6jerOC 90WKX6/Uh2pSGOo5j1QmUnjDQjKSkeyugwKmx8KkklN157BjoWl1ehj+GwL3OjPpb//3 YFEgwpbFU4NPnpDRav/gVvAEkVbXGzdBByRJIyU45bGPdZiBb5VExLjFV/3l2Pkr2lcs 1qhOUeLrb6r4O7d/2bmmOoRinvHu0mAc5MDOyZX/tQS2lpoYvYXfw7bXJnLPo8Klj5r5 JSaVSwnKhBapJCxivbOQcraHTe+qiZARAetvXLwgA3uD8tOfz3LV48iimdR7ULcrG7ka 9Wbg== X-Gm-Message-State: AOAM531xiKoroMCAjirg84RkO/s0YjeYP7S5OvqUaKJO7KEcfAvR52Jt GAfprn1JUqaB86fZn6m05EE3tA== X-Google-Smtp-Source: ABdhPJyErHJOe2eigD6Pe1OIBaMEqvLLg9JCTbRnZxDUfifjae8Oc+vn43Ix0SE6TAOgGC6ZpgDQ4w== X-Received: by 2002:a63:1542:: with SMTP id 2mr22429008pgv.329.1625601540870; Tue, 06 Jul 2021 12:59:00 -0700 (PDT) Received: from [192.168.1.108] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id j15sm17493077pfh.194.2021.07.06.12.58.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 06 Jul 2021 12:59:00 -0700 (PDT) Subject: Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems To: Stafford Horne Cc: GLIBC patches , Alistair Francis References: <20210607131851.4015661-1-shorne@gmail.com> <8ad55354-5d41-eaa4-6d6c-e650ea30d7cc@linaro.org> From: Adhemerval Zanella Message-ID: Date: Tue, 6 Jul 2021 16:58:58 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Tue, 06 Jul 2021 19:59:04 -0000 On 12/06/2021 06:19, Stafford Horne wrote: > 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); > > > Sigh... it seems that openrisc will the only *one* architecture with 64-bit time_t in userland which uses legacy 32-bit kernel ABI. > > 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; > +} > + The name is confusing, it is mixing timeval from 'struct timeval' and time_t. And there is no need to use __int32_t, we need to use it only on installed headers to avoid namespace pollution. I fact I think it would be better to just change 'in_time_t_range' to use int32_t internally instead of time_t; I am pretty sure that all usages assume that sizeof(time_t) == 32.