Message ID | 87zicvkbqw.fsf@esperi.org.uk |
---|---|
State | New, archived |
Headers |
Received: (qmail 129794 invoked by alias); 25 Jun 2017 21:18:08 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 129764 invoked by uid 89); 25 Jun 2017 21:18:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, UNPARSEABLE_RELAY autolearn=ham version=3.3.2 spammy=RESEND, nis, Etc, unlikely X-HELO: aserp1040.oracle.com From: Nick Alcock <nix@esperi.org.uk> To: libc-alpha@sourceware.org Subject: [PATCH RESEND] zic, various tests: use LFS I/O functions explicitly where needed Date: Sun, 25 Jun 2017 22:17:59 +0100 Message-ID: <87zicvkbqw.fsf@esperi.org.uk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.91 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain |
Commit Message
Nix
June 25, 2017, 9:17 p.m. UTC
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.
Comments
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.
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;