This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH RESEND] zic, various tests: use LFS I/O functions explicitly where needed
- From: Nick Alcock <nix at esperi dot org dot uk>
- To: Paul Eggert <eggert at cs dot ucla dot edu>
- Cc: libc-alpha at sourceware dot org
- Date: Mon, 26 Jun 2017 00:48:08 +0100
- Subject: Re: [PATCH RESEND] zic, various tests: use LFS I/O functions explicitly where needed
- Authentication-results: sourceware.org; auth=none
- References: <87zicvkbqw.fsf@esperi.org.uk> <b9591daf-44e4-075c-9caf-470ab84521c7@cs.ucla.edu>
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.
--
NULL && (void)