[RESEND] zic, various tests: use LFS I/O functions explicitly where needed

  From: Nick Alcock <nick.alcock@oracle.com>

This fixes this compilation failure when testing a 32-bit glibc on a
system using a 64-bit-inode XFS filesystem, and a variety of related
test failures of dirent/list, io/bug-ftw2, and posix/tst-regex:

env GCONV_PATH=/usr/src/glibc/i686-loom/iconvdata LOCPATH=/usr/src/glibc/i686-loom/localedata LC_ALL=C  /usr/src/glibc/i686-loom/elf/ld-linux.so.2 --library-path /usr/src/glibc/i686-loom:/usr/src/glibc/i686-loom/math:/usr/src/glibc/i686-loom/elf:/usr/src/glibc/i686-loom/dlfcn:/usr/src/glibc/i686-loom/nss:/usr/src/glibc/i686-loom/nis:/usr/src/glibc/i686-loom/rt:/usr/src/glibc/i686-loom/resolv:/usr/src/glibc/i686-loom/crypt:/usr/src/glibc/i686-loom/mathvec:/usr/src/glibc/i686-loom/support:/usr/src/glibc/i686-loom/nptl /usr/src/glibc/i686-loom/timezone/zic -d /usr/src/glibc/i686-loom/timezone/testdata -y ./yearistype australasia; ../scripts/evaluate-test.sh timezone/testdata/Australia/Melbourne $? false false > /usr/src/glibc/i686-loom/timezone/testdata/Australia/Melbourne.test-result
warning: "etcetera", line 13: /usr/src/glibc/i686-loom/timezone/zic: Can't create directory /usr/src: File exists
/bin/sh: /usr/src/glibc/i686-loom/timezone/testdata/Etc/UTC.test-result: No such file or directory
make[2]: *** [Makefile:98: /usr/src/glibc/i686-loom/timezone/testdata/Etc/UTC] Error 1

This happens because itsdir() in timezone/zic.c does a stat() of each
element of the path in turn, and this returns -EOVERFLOW because on this
system /usr has an inode number of 7516193792 and we did not compile zic
with -D_FILE_OFFSET_BITS=64 or use stat64().  So do the latter, as other
tools in glibc do.

We have to do similar things to fix the other test failures (though they
only appear at test-runtime, so do not cause make check to abort).

This at least allows a 32-bit libc on a 64-bit-inode fs to pass make
check for me. It may well not be the minimum required for successful
operations, though nothing else in glibc's tools is obviously
troublesome that I can see. (There is one test that calls ftw ("/")
which is unlikely to have an inode number > 2^32 on any current
filesystem, and one that calls it on a directory under /tmp which I
cannot easily test a fix for because my /tmp is a tmpfs with 32-bit
inodes. I can fix these two as well if you like, though.)

	* timezone/zic.c (itsdir): Use stat64.
	* posix/tst-regex.c (do_test): Use fstat64.
	* dirent/list.c (list): Use dirent64/readdir64.
	* io/bug-ftw.c (main): Use ftw64.
  

Nick Alcock wrote:

> -	struct stat st;
> -	int res = stat(name, &st);
> +	struct stat64 st;
> +	int res = stat64(name, &st);

If this change fixes zic, we should be compiling timezone/*.c with 
-D_FILE_OFFSET_BITS=64, as the code is imported from tzcode and the .c files 
should be left unchanged when possible.

However, I don't see why the change fixes zic, so could you please explain? The 
code after the patch looks like the following, and this should do the right 
thing even if stat(name, &st) returns -1 and sets errno to EOVERFLOW. So why 
does switching to stat64 improve zic's behavior?

#ifdef S_ISDIR
	if (res == 0)
		return S_ISDIR(st.st_mode) != 0;
#endif
	if (res == 0 || errno == EOVERFLOW) {
		size_t n = strlen(name);
		char *nameslashdot = emalloc(n + 3);
		bool dir;
		memcpy(nameslashdot, name, n);
		strcpy(&nameslashdot[n], &"/."[! (n && name[n - 1] != '/')]);
		dir = stat(nameslashdot, &st) == 0 || errno == EOVERFLOW;
		free(nameslashdot);
		return dir;
	}
	return false;
  

On 25 Jun 2017, Paul Eggert spake thusly:

> Nick Alcock wrote:
>
>> -	struct stat st;
>> -	int res = stat(name, &st);
>> +	struct stat64 st;
>> +	int res = stat64(name, &st);
>
> If this change fixes zic, we should be compiling timezone/*.c with
> -D_FILE_OFFSET_BITS=64, as the code is imported from tzcode and the .c
> files should be left unchanged when possible.

That's possible -- I tried that initially, then decided that if I
compiled any tools with -D_FILE_OFFSET_BITS=64 I should be compiling all
of them with it, then abandoned that when it turned out that this broke
a bunch of them (like memusagestat) since they already had explicit
support for stat64() as well as stat() and using FOB=64 led to link
failures.

Maybe I should have fallen back on adding to tz-cflags?

> However, I don't see why the change fixes zic, so could you please
> explain? The code after the patch looks like the following, and this
> should do the right thing even if stat(name, &st) returns -1 and sets
> errno to EOVERFLOW. So why does switching to stat64 improve zic's
> behavior?

Hm. While the other hunks in this are probably still necessary, I may
have jumped the gun with this one: it may not be needed, though I still
think it's an improvement (well, either this or adding to tz-cflags). In
glibc 2.25, the code looks like

static int
itsdir(char const *name)
{
        struct stat st;
        int res = stat(name, &st);
        if (res != 0)
                return res;
#ifdef S_ISDIR
        return S_ISDIR(st.st_mode) != 0;
#else

I spotted the problem and tested the fix in 2.25, then forward-ported
the patch (astonishingly, it applied!) and verified that the bug was
still fixed -- I did *not* verify that it still happened without the
patch, and indeed on trunk it is unnecessary. However, the new code
makes me nervous:

> #ifdef S_ISDIR
> 	if (res == 0)
> 		return S_ISDIR(st.st_mode) != 0;
> #endif
> 	if (res == 0 || errno == EOVERFLOW) {

This seems risky. We check for -EOVERFLOW, sure, but we don't check for
S_ISDIR (because we can't, because the stat failed). So in trunk, this
can conclude that something is a directory that isn't, if the kernel
checks for errors in the wrong order.

After all, if the stat() above returned -EOVERFLOW...

> 		size_t n = strlen(name);
> 		char *nameslashdot = emalloc(n + 3);
> 		bool dir;
> 		memcpy(nameslashdot, name, n);
> 		strcpy(&nameslashdot[n], &"/."[! (n && name[n - 1] != '/')]);
> 		dir = stat(nameslashdot, &st) == 0 || errno == EOVERFLOW;

... and this one also returned -EOVERFLOW, you are now depending on the
order of error-checks in the kernel's stat implementation (that it
returns EOVERFLOW only after it's had the opportunity to return ENOENT,
so that EOVERFLOW -> !ENOENT). Thi sseems, uh, a little fragile compared
to just calling stat64().

I'll try to roll a new patch updating tz-cflags tomorrow.
  

On 06/25/2017 11:17 PM, Nick Alcock wrote:
> This happens because itsdir() in timezone/zic.c does a stat() of each
> element of the path in turn, and this returns -EOVERFLOW because on this
> system /usr has an inode number of 7516193792 and we did not compile zic
> with -D_FILE_OFFSET_BITS=64 or use stat64().  So do the latter, as other
> tools in glibc do.

The zic issue looks like a user-visible bug, so please file a bug in
Bugzilla for it.  Or perhaps reuse bug 15333?

Thanks,
Florian
  

On 26 Jun 2017, Florian Weimer uttered the following:

> On 06/25/2017 11:17 PM, Nick Alcock wrote:
>> This happens because itsdir() in timezone/zic.c does a stat() of each
>> element of the path in turn, and this returns -EOVERFLOW because on this
>> system /usr has an inode number of 7516193792 and we did not compile zic
>> with -D_FILE_OFFSET_BITS=64 or use stat64().  So do the latter, as other
>> tools in glibc do.
>
> The zic issue looks like a user-visible bug, so please file a bug in
> Bugzilla for it.  Or perhaps reuse bug 15333?

I'll use 15333: it's clearly the same bug, plus a bunch of affected
tests :)
  

Nick Alcock wrote:
> you are now depending on the
> order of error-checks in the kernel's stat implementation (that it
> returns EOVERFLOW only after it's had the opportunity to return ENOENT,
> so that EOVERFLOW -> !ENOENT)

No, because ENOENT and EOVERFLOW are mutually exclusive regardless of the order 
of error checks, as the stat buffer of a nonexistent file cannot possibly 
overflow. So there is no dependency, and no bug here.

> Maybe I should have fallen back on adding to tz-cflags?

That would be better, in that glibc source would continue to match tzcode 
exactly. It doesn't hurt correctness to compile zic.c with 
-D_FILE_OFFSET_BITS=64, and doing that should make 32-bit zic run a tiny bit 
faster on directories whose inode numbers (or sizes, timestamps, ...) don't fit 
in 32 bits. So it is a tiny performance win even if it is not a bug fix.
  

On 26 Jun 2017, Paul Eggert stated:

> Nick Alcock wrote:
>> you are now depending on the
>> order of error-checks in the kernel's stat implementation (that it
>> returns EOVERFLOW only after it's had the opportunity to return ENOENT,
>> so that EOVERFLOW -> !ENOENT)
>
> No, because ENOENT and EOVERFLOW are mutually exclusive regardless of
> the order of error checks, as the stat buffer of a nonexistent file
> cannot possibly overflow. So there is no dependency, and no bug here.

Oh, true. I was thinking of an EOVERFLOW from the directory, but of
course if you get that you cannot possibly be issuing a stat for a
nonexistent file at the same time!

>> Maybe I should have fallen back on adding to tz-cflags?
>
> That would be better, in that glibc source would continue to match
> tzcode exactly. It doesn't hurt correctness to compile zic.c with
> -D_FILE_OFFSET_BITS=64, and doing that should make 32-bit zic run a
> tiny bit faster on directories whose inode numbers (or sizes,
> timestamps, ...) don't fit in 32 bits. So it is a tiny performance win
> even if it is not a bug fix.

Well, we've seen zic fail with bizarre error messages in this situation
already. So I'd call it a bugfix. :)

(Did you mean zdump, which is in tz-cflags but is not affected by this?)
  

Nick Alcock wrote:

> we've seen zic fail with bizarre error messages in this situation
> already.

I think those failures were fixed by the EOVERFLOW check that is in zic already. 
That being said, the patch shouldn't hurt correctness and should help 
performance a bit in some cases, so I just now installed a more-portable patch 
upstream. Please see:

http://mm.icann.org/pipermail/tz/2017-June/025164.html

So, an alternative to your patch would be to install this upstream patch (it's 
OK to have both patches, of course).
  

On 27 Jun 2017, Paul Eggert uttered the following:

> Nick Alcock wrote:
>
>> we've seen zic fail with bizarre error messages in this situation
>> already.
>
> I think those failures were fixed by the EOVERFLOW check that is in
> zic already.

Argh, yes, of course, what were we just *talking* about! :) of course it
is and I am talking rubbish. I guess I should change the patch subject
at least, since it's all tests now...

>              That being said, the patch shouldn't hurt correctness and
> should help performance a bit in some cases, so I just now installed a
> more-portable patch upstream. Please see:
>
> http://mm.icann.org/pipermail/tz/2017-June/025164.html

Looks fine.

> So, an alternative to your patch would be to install this upstream
> patch (it's OK to have both patches, of course).

I'll leave that decision up to you. I'm happy to drop this bit now and
make this a test-only patch.
  

Nix wrote:
> I'm happy to drop this bit now and
> make this a test-only patch.

Yes, that sounds better.