This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix 32-bit getcwd() on filesystems with 64-bit inodes.
On 26/02/2018 17:23, Nix wrote:
> On 26 Feb 2018, Adhemerval Zanella told this:
>
>> On 25/02/2018 22:06, Nick Alcock wrote:
>>> From: Nick Alcock <nick.alcock@oracle.com>
>>>
>>> When compiling a 32-bit glibc on a filesystem with 64-bit inodes,
>>> we observe failures of io/tst-getcwd-abspath:
>>>
>>> tst-getcwd-abspath.c:42: numeric comparison failure
>>> left: 75 (0x4b); from: errno
>>> right: 2 (0x2); from: ENOENT
>>> tst-getcwd-abspath.c:47: numeric comparison failure
>>> left: 75 (0x4b); from: errno
>>> right: 2 (0x2); from: ENOENT
>>> error: 2 test failures
>>>
>>> Errno 75 is EOVERFLOW, which is most unexpected from getcwd()! Having
>>> had experience with this class of pain in zic before (a patch which I
>>> should perhaps resubmit or combine with this one), the cause is clear:
>>> the getcwd() implementation was calling readdir(), and in glibc that is
>>> the non-LFS implementation so it falls over with just that error if it
>>> sees a single 64-bit inode.
>>
>> Thanks for checking on it and I see no reason to continue using non-LFS
>> calls on loader for 32-bits architectures.
>
> Neither do I. There are quite a lot of non-LFS calls elsewhere (I have
> another patch that purges a bunch of them from other tests, for
> instance, but they're even less related to this bug than the ttyname
> stuff in here was).
>
>> I don't see this regression with a i686-linux-gnu build running on a
>> x86_64 kernel, so it seems the case where the system configuration is
>> interfering on the testcases. It would be good if we could isolate such
>> issues, so do you have any information why exactly getdents is failing?
>
> You need a filesystem on which inode numbers are routinely > 2^32, for
> instance a >1TiB XFS filesystem mounted without the
> (performance-destroying) inode32 option. On such filesystems, getdents()
> (but not getdents64()) will return -EOVERFLOW if any inode number in the
> set to be returned will not fit in the 32-bit space of an ino_t. (See
> the various references to -EOVERFLOW in fs/readdir.c, which handle one
> instance or another of this.)
>
> This happens on xfs because it uses physical disk block index as an
> inode number: ext4 etc, which use more conventional inode tables, always
> has sub-32-bit inode numbers except in the largest of filesystems and so
> will not reveal this problem. But XFS is getting more common these days,
> and big filesystems are too... you'll also see it on NFSv4 mounts to
> such a kernel (and if youre using NFS, you'll be using NFSv4: NFSv3
> mounts will generally not work right because the protocol cannot handle
> 64-bit inode numbers).
Thanks for thoroughly explanation, this explain why I am not seeing this
issue in any of environments I have access.
>
>>> getcwd() is used in the dynamic linker as part of $ORIGIN support, so
>>> the usual SHLIB_COMPAT dance is needed there to prevent versioned symbols
>>> getting into it and causing disaster.
>>>
>>> While we're at it, fix likely similar errors in ttyname() (determined
>>> by code inspection, since my /dev is a tmpfs with 32-bit indoes: but
>>> one could run glibc on a system with a 64-bit-inode filesystem and
>>> a static /dev, and then this would probably fail too, the same way.)
>>
>> I will recommend to spit the ttyname fix on its own patch (and Linux has
>> its own versio which already uses LFS calls already).
>
> Oh, does it? I missed that, The ttyname part may be entirely unnecessary
> then. I'll just drop it for now: if it belongs anywhere it's not here,
> and I have no proof it causes any trouble at all (unlike getcwd).
Linux uses sysdeps/unix/sysv/linux/ttyname.c and it does not use any
fallback implementation as getcwd.
>
>>>
>>> * include/dirent.h: Make __readdir64 hidden in rtld too.
>>> * sysdeps/unix/sysv/linux/i386/readdir64.c: Mark SHLIB_COMPAT
>>> in libc only.
>>
>> Indentation seems a bit off here with extra spaces.
>
> That's what happens when you forget an M-x tabify. :/
>
>>> It is quite possible I'm being unidiomatic in include/dirent.h and
>>> sysdeps/unix/sysv/linux/i386/readdir64.c: I'd be happy to switch to
>>> the idiomatic approach if someone could tell me what it is. :)
>>
>> The readdir LFS consolidation to avoid the multiple version for the
>> different architecture we have is on my backlog for some time. I think
>> current approach is fine.
>
> Thanks!
>
>>> diff --git a/include/dirent.h b/include/dirent.h
>>> index caaeb0be85..c1d3c6b53f 100644
>>> --- a/include/dirent.h
>>> +++ b/include/dirent.h
>>> @@ -21,7 +21,9 @@ extern DIR *__fdopendir (int __fd) attribute_hidden;
>>> extern int __closedir (DIR *__dirp) attribute_hidden;
>>> extern struct dirent *__readdir (DIR *__dirp) attribute_hidden;
>>> extern struct dirent64 *__readdir64 (DIR *__dirp);
>>> -libc_hidden_proto (__readdir64)
>>> +# if IS_IN (libc) || IS_IN (rtld)
>>> + hidden_proto (__readdir64)
>>> +# endif
>>
>> I think you will need a '&& !defined NO_RTLD_HIDDEN' for Hurd (I can't confirm
>> even with build-many-glibc.py Hurd is missing the thread configuration to
>> complete the build).
>
> It does look like it, from other uses. I'll add that, drop the ttyname
> stuff for now, open a bug, and resubmit.
>