Re: [PATCH 15/16] linux: Add {f}stat{at} y2038 support

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5074 bytes --]



On 13/10/2020 11:18, Adhemerval Zanella wrote:
> 
> 
> On 13/10/2020 10:58, Lukasz Majewski wrote:
>> Hi Adhemerval,
>>
>>> On 07/10/2020 09:52, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 06/10/2020 06:48, Lukasz Majewski wrote:  
>>>>> Hi Adhemerval,
>>>>>  
>>>>>> A new struct __stat{64}_t64 type is added with the required
>>>>>> __timespec64 time definition.  Both non-LFS and LFS support were
>>>>>> done with an extra __NR_statx call plus a conversion to the new
>>>>>> __stat{64}_t64 type.  The statx call is done only for
>>>>>> architectures with support for 32-bit time_t ABI.
>>>>>>
>>>>>> Internally some extra routines to copy from/to struct stat{64}
>>>>>> to struct __stat{64} used on multiple implementations (stat,
>>>>>> fstat, lstat, and fstatat) are added on a extra file
>>>>>> (stat_t64_cp.c).  Aslo some extra routines to copy from statx to
>>>>>> __stat{64} is added on statx_cp.c.
>>>>>>
>>>>>> Checked with a build for all affected ABIs. I also checked on
>>>>>> x86_64, i686, powerpc, powerpc64le, sparcv9, sparc64, s390, and
>>>>>> s390x.  
>>>>>
>>>>> When do you plan to pull this patch set to -master?
>>>>> Those patches have been available for review on the mailing list
>>>>> for more than two months now.  
>>>>
>>>> Hi Lukasz, thanks to remind me. I will rebase against master and run
>>>> some regressions tests against some platforms and push it.
>>>>   
>>>
>>> One required change with the rebase is adapt the riscv32 ABI to
>>> exclude the __{f,l}xstat{at} symbol and replace with proper {f,l}stat
>>> ones. It is possible because the new ABI was added on current
>>> development branch, however one minor inconvenient is the toolchain
>>> need to be rebuild with a updated glibc branch to avoid linking
>>> failures with libstd++ (which uses __{f,l}xstat{at}).
>>>
>>
>> I'm not sure if this is related, but on my ARMv7 (32 bit) sandbox there
>> is an issue with fstat accesses to files.
>>
>> When I try to run a program build against newest glibc (installed in
>> /opt/lib) I do see issues with {f}stat on other libraries (e.g.
>> /opt/lib/librt.so). To be more specific I do experience the EOVERFLOW
>> error:
>>
>> error while loading shared libraries: librt.so.1: cannot stat shared
>> object: Error 75
>>
>> The "base" glibc is 2.28 (installed in /lib). The glibc under test is
>> the newest master installed in /opt/lib.
>>
>> I'm now investigating this issue.
> 
> I am not sure what it might be based on these information, could you
> provide a strace so we can pinpoint what might the issue?  
> 
> The arm-linux-gnueabihf testing I did was on a aarch64 kernel (4.12.13).
> Besides the make check without regression, I could run system binaries 
> with ./testrun.sh.
> 
> I will check on a different kernel/system with a 32-bit kernel.

Ok, this change in fact triggered a very subtle issue at dl-load.c that
I saw in both arm-linux-gnueabihf system with a 32-bit kernel and on
mips-linux-gnu.

The issue is at:

elf/dl-load.c

1982       if (here_any && (err = errno) != ENOENT && err != EACCES)
1983         /* The file exists and is readable, but something went wrong.  */
1984         return -1;

And it is just triggered on system where {f,l}stat{at}{64} issues
__NR_statx and it fails with ENOSYS but later success with the system
stat* syscall.

This code here checks the errno value without checking whether the
previous function call that might change err actually has failed
(in this specific case the stat64 at line 1931). And this due how we 
currently implement the y2038 support with INLINE_SYSCALL_CALL 
(a function that succeeds is allowed to change errno and it simplifies 
the resulting y2038 support a bit).

In fact this check does not really make sense, since either 'fd' will
be different than '0' (meaning it has being opened) or the 'stat64'
at line 1931 failed and 'here_any' won't be set (the stat64 at line
1951 already explicit sets errno in failure case).  

Also, git history does not give much information on why it was added
at fist place.  So I think we just need to remove this extra check,
you can check if the following patch helps (I am running some
regression tests before sensing it upstream):

diff --git a/elf/dl-load.c b/elf/dl-load.c
index f3201e7c14..39ae43c6ce 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1878,7 +1878,6 @@ open_path (const char *name, size_t namelen, int mode,
       size_t cnt;
       char *edp;
       int here_any = 0;
-      int err;
 
       /* If we are debugging the search for libraries print the path
         now if it hasn't happened now.  */
@@ -1979,9 +1978,6 @@ open_path (const char *name, size_t namelen, int mode,
              return -1;
            }
        }
-      if (here_any && (err = errno) != ENOENT && err != EACCES)
-       /* The file exists and is readable, but something went wrong.  */
-       return -1;
 
       /* Remember whether we found anything.  */
       any |= here_any;





[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]