[PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Aurelien Jarno]

Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
stop calling it from __fstatat for systems with 64-bit time_t.

Resolves: BZ #29730
---
 sysdeps/unix/sysv/linux/fstatat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/fstatat.c b/sysdeps/unix/sysv/linux/fstatat.c
index 055fb4762e..afe93b6cf4 100644
--- a/sysdeps/unix/sysv/linux/fstatat.c
+++ b/sysdeps/unix/sysv/linux/fstatat.c
@@ -33,9 +33,13 @@ __fstatat (int fd, const char *file, struct stat *buf, int flag)
       if (! in_ino_t_range (st64.st_ino)
 	  || ! in_off_t_range (st64.st_size)
 	  || ! in_blkcnt_t_range (st64.st_blocks)
+/* in_time_t_range assumes that all usages are for sizeof(time_t) == 32.  */
+# if __TIMESIZE != 64
 	  || ! in_time_t_range (st64.st_atim.tv_sec)
 	  || ! in_time_t_range (st64.st_mtim.tv_sec)
-	  || ! in_time_t_range (st64.st_ctim.tv_sec))
+	  || ! in_time_t_range (st64.st_ctim.tv_sec)
+# endif
+	 )
 	return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
 
       /* Clear internal pad and reserved fields.  */
-- 
2.35.1

Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Adhemerval Zanella Netto]

On 28/10/22 17:54, Aurelien Jarno wrote:
> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
> stop calling it from __fstatat for systems with 64-bit time_t.
> 
> Resolves: BZ #29730

Although this change is not incorrect, I think the fix I just sent [1] is 
slight better because it just alias non-LFS stat to LFS stat for mips 
(which is also a small runtime and code optimization), while avoiding 
touching generic code (so we need to check if trigger another issue on 
different architecture).

[1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html

> ---
>  sysdeps/unix/sysv/linux/fstatat.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/fstatat.c b/sysdeps/unix/sysv/linux/fstatat.c
> index 055fb4762e..afe93b6cf4 100644
> --- a/sysdeps/unix/sysv/linux/fstatat.c
> +++ b/sysdeps/unix/sysv/linux/fstatat.c
> @@ -33,9 +33,13 @@ __fstatat (int fd, const char *file, struct stat *buf, int flag)
>        if (! in_ino_t_range (st64.st_ino)
>  	  || ! in_off_t_range (st64.st_size)
>  	  || ! in_blkcnt_t_range (st64.st_blocks)
> +/* in_time_t_range assumes that all usages are for sizeof(time_t) == 32.  */
> +# if __TIMESIZE != 64
>  	  || ! in_time_t_range (st64.st_atim.tv_sec)
>  	  || ! in_time_t_range (st64.st_mtim.tv_sec)
> -	  || ! in_time_t_range (st64.st_ctim.tv_sec))
> +	  || ! in_time_t_range (st64.st_ctim.tv_sec)
> +# endif
> +	 )
>  	return INLINE_SYSCALL_ERROR_RETURN_VALUE (EOVERFLOW);
>  
>        /* Clear internal pad and reserved fields.  */

Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Adhemerval Zanella Netto]

On 31/10/22 13:03, Adhemerval Zanella Netto wrote:
> 
> 
> On 28/10/22 17:54, Aurelien Jarno wrote:
>> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
>> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
>> stop calling it from __fstatat for systems with 64-bit time_t.
>>
>> Resolves: BZ #29730
> 
> Although this change is not incorrect, I think the fix I just sent [1] is 
> slight better because it just alias non-LFS stat to LFS stat for mips 
> (which is also a small runtime and code optimization), while avoiding 
> touching generic code (so we need to check if trigger another issue on 
> different architecture).
> 
> [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html

So I was wrong that we can assume stat and stat64 are equal on mips.  Maybe
instead of adding the fix on stat we can make it more generic and move it
to is_time_t_range (so it can potentially fix usage not only on stat code)
as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ?

Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Aurelien Jarno]

On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 31/10/22 13:03, Adhemerval Zanella Netto wrote:
> > 
> > 
> > On 28/10/22 17:54, Aurelien Jarno wrote:
> >> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
> >> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
> >> stop calling it from __fstatat for systems with 64-bit time_t.
> >>
> >> Resolves: BZ #29730
> > 
> > Although this change is not incorrect, I think the fix I just sent [1] is 
> > slight better because it just alias non-LFS stat to LFS stat for mips 
> > (which is also a small runtime and code optimization), while avoiding 
> > touching generic code (so we need to check if trigger another issue on 
> > different architecture).
> > 
> > [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html
> 
> So I was wrong that we can assume stat and stat64 are equal on mips.  Maybe
> instead of adding the fix on stat we can make it more generic and move it
> to is_time_t_range (so it can potentially fix usage not only on stat code)
> as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ?

Yes, we can move it to is_time_t_range, but then it gets trickier to get
the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it
is not defined for OpenRISC, so it means it will cast to time_t. However
this has been explicitly changed that way in commit 6e8a0aac2f883 to
unbreak OpenRISC.

We probably want a condition that is "kernel ABI uses 32-bit time_t". Do
we have something like that already defined?

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Aurelien Jarno]

On 2022-10-31 23:20, Aurelien Jarno wrote:
> On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote:
> > 
> > 
> > On 31/10/22 13:03, Adhemerval Zanella Netto wrote:
> > > 
> > > 
> > > On 28/10/22 17:54, Aurelien Jarno wrote:
> > >> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
> > >> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
> > >> stop calling it from __fstatat for systems with 64-bit time_t.
> > >>
> > >> Resolves: BZ #29730
> > > 
> > > Although this change is not incorrect, I think the fix I just sent [1] is 
> > > slight better because it just alias non-LFS stat to LFS stat for mips 
> > > (which is also a small runtime and code optimization), while avoiding 
> > > touching generic code (so we need to check if trigger another issue on 
> > > different architecture).
> > > 
> > > [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html
> > 
> > So I was wrong that we can assume stat and stat64 are equal on mips.  Maybe
> > instead of adding the fix on stat we can make it more generic and move it
> > to is_time_t_range (so it can potentially fix usage not only on stat code)
> > as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ?
> 
> Yes, we can move it to is_time_t_range, but then it gets trickier to get
> the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it
> is not defined for OpenRISC, so it means it will cast to time_t. However
> this has been explicitly changed that way in commit 6e8a0aac2f883 to
> unbreak OpenRISC.
> 
> We probably want a condition that is "kernel ABI uses 32-bit time_t". Do
> we have something like that already defined?

I guess this can be checked with:
(__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))

And then we can rename the function to is_ktime_t_range to better
reflect its behaviour.


-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Aurelien Jarno]

On 2022-11-01 11:55, Aurelien Jarno wrote:
> On 2022-10-31 23:20, Aurelien Jarno wrote:
> > On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote:
> > > 
> > > 
> > > On 31/10/22 13:03, Adhemerval Zanella Netto wrote:
> > > > 
> > > > 
> > > > On 28/10/22 17:54, Aurelien Jarno wrote:
> > > >> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
> > > >> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
> > > >> stop calling it from __fstatat for systems with 64-bit time_t.
> > > >>
> > > >> Resolves: BZ #29730
> > > > 
> > > > Although this change is not incorrect, I think the fix I just sent [1] is 
> > > > slight better because it just alias non-LFS stat to LFS stat for mips 
> > > > (which is also a small runtime and code optimization), while avoiding 
> > > > touching generic code (so we need to check if trigger another issue on 
> > > > different architecture).
> > > > 
> > > > [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html
> > > 
> > > So I was wrong that we can assume stat and stat64 are equal on mips.  Maybe
> > > instead of adding the fix on stat we can make it more generic and move it
> > > to is_time_t_range (so it can potentially fix usage not only on stat code)
> > > as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ?
> > 
> > Yes, we can move it to is_time_t_range, but then it gets trickier to get
> > the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it
> > is not defined for OpenRISC, so it means it will cast to time_t. However
> > this has been explicitly changed that way in commit 6e8a0aac2f883 to
> > unbreak OpenRISC.
> > 
> > We probably want a condition that is "kernel ABI uses 32-bit time_t". Do
> > we have something like that already defined?
> 
> I guess this can be checked with:
> (__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
> 
> And then we can rename the function to is_ktime_t_range to better
> reflect its behaviour.

Looking at it more in details, I am not sure it's a good idea, as this
function is also used for non-syscall cases, like in mktime or timegm.
While that works because there is a __TIMESIZE != 64 guard, this still
seems fragile.

In that regard, I think the approach from YunQiang to explicitly call
either one or the other function might be better.

An hybrid solution would be to define both in_time_t_range and
in_ktime_t_range, the later being the one called in_int32_t_range in
YunQiang's patch.

What do you think?

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Adhemerval Zanella Netto]

On 01/11/22 08:57, Aurelien Jarno wrote:
> On 2022-11-01 11:55, Aurelien Jarno wrote:
>> On 2022-10-31 23:20, Aurelien Jarno wrote:
>>> On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote:
>>>>
>>>>
>>>> On 31/10/22 13:03, Adhemerval Zanella Netto wrote:
>>>>>
>>>>>
>>>>> On 28/10/22 17:54, Aurelien Jarno wrote:
>>>>>> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
>>>>>> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
>>>>>> stop calling it from __fstatat for systems with 64-bit time_t.
>>>>>>
>>>>>> Resolves: BZ #29730
>>>>>
>>>>> Although this change is not incorrect, I think the fix I just sent [1] is 
>>>>> slight better because it just alias non-LFS stat to LFS stat for mips 
>>>>> (which is also a small runtime and code optimization), while avoiding 
>>>>> touching generic code (so we need to check if trigger another issue on 
>>>>> different architecture).
>>>>>
>>>>> [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html
>>>>
>>>> So I was wrong that we can assume stat and stat64 are equal on mips.  Maybe
>>>> instead of adding the fix on stat we can make it more generic and move it
>>>> to is_time_t_range (so it can potentially fix usage not only on stat code)
>>>> as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ?
>>>
>>> Yes, we can move it to is_time_t_range, but then it gets trickier to get
>>> the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it
>>> is not defined for OpenRISC, so it means it will cast to time_t. However
>>> this has been explicitly changed that way in commit 6e8a0aac2f883 to
>>> unbreak OpenRISC.
>>>
>>> We probably want a condition that is "kernel ABI uses 32-bit time_t". Do
>>> we have something like that already defined?
>>
>> I guess this can be checked with:
>> (__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
>>
>> And then we can rename the function to is_ktime_t_range to better
>> reflect its behaviour.
> 
> Looking at it more in details, I am not sure it's a good idea, as this
> function is also used for non-syscall cases, like in mktime or timegm.
> While that works because there is a __TIMESIZE != 64 guard, this still
> seems fragile.

The "(__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))"
is a Linuxism and I agree it should not be used in generic code.

> 
> In that regard, I think the approach from YunQiang to explicitly call
> either one or the other function might be better.
> 
> An hybrid solution would be to define both in_time_t_range and
> in_ktime_t_range, the later being the one called in_int32_t_range in
> YunQiang's patch.
> 
> What do you think?
> 

I am more inclined to either go with your initial patch that handles it on
fstatat or go with a mips specific fstatat.c to make clear about MIPS ABI
idiosyncrasies (which is something I tried to avoid, but for this specific
case seems to make sense):

* sysdeps/unix/sysv/linux/mips/mips64/n64/fstatat.c:

/* Different than other ABIS, mips64 has different layouts for non-LFS
   and LFS struct stat.  */
int
__fstatat (int fd, const char *file, struct stat *buf, int flag)
{
  struct __stat64_t64 st64;
  int r = __fstatat64_time64 (fd, file, &st64, flag);
  if (r == 0)
    {
      /* Clear internal pad and reserved fields.  */
      memset (buf, 0, sizeof (*buf));

      buf->st_dev = st64.st_dev;
      buf->st_ino = st64.st_ino;
      buf->st_mode = st64.st_mode;
      buf->st_nlink = st64.st_nlink;
      buf->st_uid = st64.st_uid;
      buf->st_gid = st64.st_gid;
      buf->st_rdev = st64.st_rdev;
      buf->st_size = st64.st_size;
      buf->st_blksize = st64.st_blksize;
      buf->st_blocks  = st64.st_blocks;
      buf->st_atim = st64.st_atim;
      buf->st_mtim = st64.st_mtim;
      buf->st_ctim = st64.st_ctim;
    }
  return r;
}
 
We can then treat the YunQiang patch as a code refactor instead.

Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Aurelien Jarno]

On 2022-11-01 09:40, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> On 01/11/22 08:57, Aurelien Jarno wrote:
> > On 2022-11-01 11:55, Aurelien Jarno wrote:
> >> On 2022-10-31 23:20, Aurelien Jarno wrote:
> >>> On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote:
> >>>>
> >>>>
> >>>> On 31/10/22 13:03, Adhemerval Zanella Netto wrote:
> >>>>>
> >>>>>
> >>>>> On 28/10/22 17:54, Aurelien Jarno wrote:
> >>>>>> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
> >>>>>> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
> >>>>>> stop calling it from __fstatat for systems with 64-bit time_t.
> >>>>>>
> >>>>>> Resolves: BZ #29730
> >>>>>
> >>>>> Although this change is not incorrect, I think the fix I just sent [1] is 
> >>>>> slight better because it just alias non-LFS stat to LFS stat for mips 
> >>>>> (which is also a small runtime and code optimization), while avoiding 
> >>>>> touching generic code (so we need to check if trigger another issue on 
> >>>>> different architecture).
> >>>>>
> >>>>> [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html
> >>>>
> >>>> So I was wrong that we can assume stat and stat64 are equal on mips.  Maybe
> >>>> instead of adding the fix on stat we can make it more generic and move it
> >>>> to is_time_t_range (so it can potentially fix usage not only on stat code)
> >>>> as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ?
> >>>
> >>> Yes, we can move it to is_time_t_range, but then it gets trickier to get
> >>> the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it
> >>> is not defined for OpenRISC, so it means it will cast to time_t. However
> >>> this has been explicitly changed that way in commit 6e8a0aac2f883 to
> >>> unbreak OpenRISC.
> >>>
> >>> We probably want a condition that is "kernel ABI uses 32-bit time_t". Do
> >>> we have something like that already defined?
> >>
> >> I guess this can be checked with:
> >> (__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
> >>
> >> And then we can rename the function to is_ktime_t_range to better
> >> reflect its behaviour.
> > 
> > Looking at it more in details, I am not sure it's a good idea, as this
> > function is also used for non-syscall cases, like in mktime or timegm.
> > While that works because there is a __TIMESIZE != 64 guard, this still
> > seems fragile.
> 
> The "(__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))"
> is a Linuxism and I agree it should not be used in generic code.
> 
> > 
> > In that regard, I think the approach from YunQiang to explicitly call
> > either one or the other function might be better.
> > 
> > An hybrid solution would be to define both in_time_t_range and
> > in_ktime_t_range, the later being the one called in_int32_t_range in
> > YunQiang's patch.
> > 
> > What do you think?
> > 
> 
> I am more inclined to either go with your initial patch that handles it on
> fstatat or go with a mips specific fstatat.c to make clear about MIPS ABI
> idiosyncrasies (which is something I tried to avoid, but for this specific
> case seems to make sense):
> 
> * sysdeps/unix/sysv/linux/mips/mips64/n64/fstatat.c:
> 
> /* Different than other ABIS, mips64 has different layouts for non-LFS
>    and LFS struct stat.  */
> int
> __fstatat (int fd, const char *file, struct stat *buf, int flag)
> {
>   struct __stat64_t64 st64;
>   int r = __fstatat64_time64 (fd, file, &st64, flag);
>   if (r == 0)
>     {
>       /* Clear internal pad and reserved fields.  */
>       memset (buf, 0, sizeof (*buf));
> 
>       buf->st_dev = st64.st_dev;
>       buf->st_ino = st64.st_ino;
>       buf->st_mode = st64.st_mode;
>       buf->st_nlink = st64.st_nlink;
>       buf->st_uid = st64.st_uid;
>       buf->st_gid = st64.st_gid;
>       buf->st_rdev = st64.st_rdev;
>       buf->st_size = st64.st_size;
>       buf->st_blksize = st64.st_blksize;
>       buf->st_blocks  = st64.st_blocks;
>       buf->st_atim = st64.st_atim;
>       buf->st_mtim = st64.st_mtim;
>       buf->st_ctim = st64.st_ctim;
>     }
>   return r;
> }
>  
> We can then treat the YunQiang patch as a code refactor instead.

Sounds good. I have tested that solution and confirm it works fine. I
have just sent a proper patch for that.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net