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

login
register
mail settings
Submitter Nix
Date June 25, 2017, 9:17 p.m.
Message ID <87zicvkbqw.fsf@esperi.org.uk>
Download mbox | patch
Permalink /patch/21258/
State New
Headers show

Comments

Nix - June 25, 2017, 9:17 p.m.
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.
Paul Eggert - June 25, 2017, 10:17 p.m.
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;
Nix - June 25, 2017, 11:48 p.m.
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.
Florian Weimer - June 26, 2017, 6:47 a.m.
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
Nix - June 26, 2017, 10:32 a.m.
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 :)
Paul Eggert - June 26, 2017, 9:08 p.m.
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.
Nix - June 26, 2017, 10:23 p.m.
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?)
Paul Eggert - June 26, 2017, 11:17 p.m.
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).
Nix - June 27, 2017, 11:34 a.m.
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.
Paul Eggert - June 28, 2017, 7:01 a.m.
Nix wrote:
> I'm happy to drop this bit now and
> make this a test-only patch.

Yes, that sounds better.

Patch

diff --git a/dirent/list.c b/dirent/list.c
index e99b253808..f792084243 100644
--- a/dirent/list.c
+++ b/dirent/list.c
@@ -26,7 +26,7 @@  static int
 test (const char *name)
 {
   DIR *dirp;
-  struct dirent *entp;
+  struct dirent64 *entp;
   int retval = 0;
 
   puts (name);
@@ -39,7 +39,7 @@  test (const char *name)
     }
 
   errno = 0;
-  while ((entp = readdir (dirp)) != NULL)
+  while ((entp = readdir64 (dirp)) != NULL)
     printf ("%s\tfile number %lu\n",
 	    entp->d_name, (unsigned long int) entp->d_fileno);
 
diff --git a/io/bug-ftw2.c b/io/bug-ftw2.c
index ce8823d28e..d6cc6a160d 100644
--- a/io/bug-ftw2.c
+++ b/io/bug-ftw2.c
@@ -69,7 +69,7 @@  main (void)
 {
   mtrace ();
 
-  ftw (".", callback, 10);
+  ftw64 (".", callback, 10);
 
   if (! sawcur)
     {
diff --git a/posix/tst-regex.c b/posix/tst-regex.c
index df2c108be5..ab9a854d98 100644
--- a/posix/tst-regex.c
+++ b/posix/tst-regex.c
@@ -57,7 +57,7 @@  do_test (void)
 {
   const char *file;
   int fd;
-  struct stat st;
+  struct stat64 st;
   int result;
   char *inmem;
   char *outmem;
@@ -72,7 +72,7 @@  do_test (void)
   if (fd == -1)
     error (EXIT_FAILURE, errno, "cannot open %s", basename (file));
 
-  if (fstat (fd, &st) != 0)
+  if (fstat64 (fd, &st) != 0)
     error (EXIT_FAILURE, errno, "cannot stat %s", basename (file));
   memlen = st.st_size;
 
diff --git a/timezone/zic.c b/timezone/zic.c
index 068fb43318..ae3e8dc041 100644
--- a/timezone/zic.c
+++ b/timezone/zic.c
@@ -950,8 +950,8 @@  static const zic_t early_time = (WORK_AROUND_GNOME_BUG_730332
 static bool
 itsdir(char const *name)
 {
-	struct stat st;
-	int res = stat(name, &st);
+	struct stat64 st;
+	int res = stat64(name, &st);
 #ifdef S_ISDIR
 	if (res == 0)
 		return S_ISDIR(st.st_mode) != 0;