Re: [PATCH] Fix 32-bit getcwd() on filesystems with 64-bit inodes.

[Nix <nix at esperi dot org dot uk>]

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).

>> 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).

>> 
>> 	* 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.

-- 
NULL && (void)