Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(539)

Issue 119530044: code review 119530044: syscall: ignore EINVAL/ENOENT from readdirent on OS X 10.10 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by rsc
Modified:
10 years, 9 months ago
Reviewers:
r, gobot, 0intro, walkeraj, bradfitz
CC:
golang-codereviews, bradfitz, dsymonds, dave_cheney.net, r, iant
Visibility:
Public.

Description

syscall: ignore EINVAL/ENOENT from readdirent on OS X 10.10 On OS X 10.10 Yosemite, if you have a directory that can be returned in a single getdirentries64 call (for example, a directory with one file), and you read from the directory at EOF twice, you get EOF both times: fd = open("dir") getdirentries64(fd) returns data getdirentries64(fd) returns 0 (EOF) getdirentries64(fd) returns 0 (EOF) But if you remove the file in the middle between the two calls, the second call returns an error instead. fd = open("dir") getdirentries64(fd) returns data getdirentries64(fd) returns 0 (EOF) remove("dir/file") getdirentries64(fd) returns ENOENT/EINVAL Whether you get ENOENT or EINVAL depends on exactly what was in the directory. It is deterministic, just data-dependent. This only happens in small directories. A directory containing more data than fits in a 4k getdirentries64 call will return EOF correctly. (It's not clear if the criteria is that the directory be split across multiple getdirentries64 calls or that it be split across multiple file system blocks.) We could change package os to avoid the second read at EOF, and maybe we should, but that's a bit involved. For now, treat the EINVAL/ENOENT as EOF. With this CL, all.bash passes on my MacBook Air running OS X 10.10 (14A299l) and Xcode 6 beta 5 (6A279r). I tried filing an issue with Apple using "Feedback Assistant", but it was unable to send the report and lost it. C program reproducing the issue, also at http://swtch.com/~rsc/readdirbug.c: #include <stdio.h> #include <dirent.h> #include <unistd.h> #include <sys/stat.h> #include <stdlib.h> #include <fcntl.h> #include <errno.h> #include <string.h> static void test(int); int main(void) { int fd, n; DIR *dir; struct dirent *dp; struct stat st; char buf[10000]; long basep; int saw; if(stat("/tmp/readdirbug", &st) >= 0) { fprintf(stderr, "please rm -r /tmp/readdirbug and run again\n"); exit(1); } fprintf(stderr, "mkdir /tmp/readdirbug\n"); if(mkdir("/tmp/readdirbug", 0777) < 0) { perror("mkdir /tmp/readdirbug"); exit(1); } fprintf(stderr, "create /tmp/readdirbug/file1\n"); if((fd = creat("/tmp/readdirbug/file1", 0666)) < 0) { perror("create /tmp/readdirbug/file1"); exit(1); } close(fd); test(0); test(1); fprintf(stderr, "ok - everything worked\n"); } static void test(int doremove) { DIR *dir; struct dirent *dp; int numeof; fprintf(stderr, "\n"); fprintf(stderr, "opendir /tmp/readdirbug\n"); dir = opendir("/tmp/readdirbug"); if(dir == 0) { perror("open /tmp/readdirbug"); exit(1); } numeof = 0; for(;;) { errno = 0; dp = readdir(dir); if(dp != 0) { fprintf(stderr, "readdir: found %s\n", dp->d_name); continue; } if(errno != 0) { perror("readdir"); exit(1); } fprintf(stderr, "readdir: EOF\n"); if(++numeof == 3) break; if(doremove) { fprintf(stderr, "rm /tmp/readdirbug/file1\n"); if(remove("/tmp/readdirbug/file1") < 0) { perror("remove"); exit(1); } } } fprintf(stderr, "closedir\n"); closedir(dir); } Fixes issue 8423.

Patch Set 1 #

Patch Set 2 : diff -r fb650123210102c630f63b3d7ab40edd05840d8f https://code.google.com/p/go #

Patch Set 3 : diff -r fb650123210102c630f63b3d7ab40edd05840d8f https://code.google.com/p/go #

Total comments: 1

Patch Set 4 : diff -r 6700bc3c932e8e488267c02c2f325e2d0e8887db https://code.google.com/p/go #

Patch Set 5 : diff -r 6700bc3c932e8e488267c02c2f325e2d0e8887db https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -1 line) Patch
M src/pkg/syscall/syscall_bsd.go View 1 2 3 1 chunk +34 lines, -1 line 0 comments Download

Messages

Total messages: 22
rsc
Hello golang-codereviews@googlegroups.com (cc: iant, r), I'd like you to review this change to https://code.google.com/p/go
10 years, 9 months ago (2014-08-07 01:52:17 UTC) #1
bradfitz
LGTM Sigh. On Aug 6, 2014 6:52 PM, <rsc@golang.org> wrote: > Reviewers: golang-codereviews, > > ...
10 years, 9 months ago (2014-08-07 01:55:19 UTC) #2
dsymonds
Feels like a bit of a dangerous workaround. What if those error codes are coming ...
10 years, 9 months ago (2014-08-07 02:00:19 UTC) #3
bradfitz
Yeah, that sounds safer. Is RemoveAll the only thing hitting this problem? We could only ...
10 years, 9 months ago (2014-08-07 02:04:33 UTC) #4
rsc
This is simple, and it works now where the current code does not. If we ...
10 years, 9 months ago (2014-08-07 02:06:20 UTC) #5
dave_cheney.net
I'm erring on the side of caution with David. Yosemite isn't released yet, although I ...
10 years, 9 months ago (2014-08-07 02:09:36 UTC) #6
bradfitz
Ideally we'd understand the bug before putting anything in 1.3.1. On Aug 6, 2014 7:06 ...
10 years, 9 months ago (2014-08-07 02:16:05 UTC) #7
bradfitz
https://codereview.appspot.com/119530044/diff/40001/src/pkg/syscall/syscall_bsd.go File src/pkg/syscall/syscall_bsd.go (right): https://codereview.appspot.com/119530044/diff/40001/src/pkg/syscall/syscall_bsd.go#newcode81 src/pkg/syscall/syscall_bsd.go:81: // that Apple's C readdir(3) uses that system call. ...
10 years, 9 months ago (2014-08-07 04:57:56 UTC) #8
rsc
I understand David's theory, but it's not true. I have tested that RemoveAll works fine ...
10 years, 9 months ago (2014-08-07 12:01:31 UTC) #9
rsc
I am happy to run more tests if anyone else has theories about what might ...
10 years, 9 months ago (2014-08-08 12:58:37 UTC) #10
bradfitz
Have you used dtrace or ktruss or whatever to see what a C program does ...
10 years, 9 months ago (2014-08-08 13:06:24 UTC) #11
rsc
On Fri, Aug 8, 2014 at 9:06 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Have you ...
10 years, 9 months ago (2014-08-08 14:04:28 UTC) #12
rsc
By the way, one difference between Go and C is that Go uses threads differently ...
10 years, 9 months ago (2014-08-08 14:05:16 UTC) #13
bradfitz
Thanks for the info. I don't object to it being submitted. Try filing a futile ...
10 years, 9 months ago (2014-08-08 14:12:56 UTC) #14
r
LGTM
10 years, 9 months ago (2014-08-08 14:22:53 UTC) #15
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=7414af99c2bf *** syscall: ignore EINVAL/ENOENT from readdirent on OS X 10.10 On ...
10 years, 9 months ago (2014-08-08 15:20:51 UTC) #16
gobot
This CL appears to have broken the netbsd-amd64-bsiegert builder. See http://build.golang.org/log/53b1000fbea15260ac4e73361194be43655fbeff
10 years, 9 months ago (2014-08-08 15:36:58 UTC) #17
rsc
Managed to reproduce in pure C using opendir/readdir. It's the second read at EOF that ...
10 years, 9 months ago (2014-08-08 15:53:22 UTC) #18
0intro
I believe you should be able to fill a bug report here: https://www.apple.com/feedback/macosx.html -- David ...
10 years, 9 months ago (2014-08-08 16:03:05 UTC) #19
rsc
On Fri, Aug 8, 2014 at 12:03 PM, David du Colombier <0intro@gmail.com> wrote: > I ...
10 years, 9 months ago (2014-08-08 17:38:58 UTC) #20
walkeraj
On 2014/08/08 17:38:58, rsc wrote: > On Fri, Aug 8, 2014 at 12:03 PM, David ...
10 years, 9 months ago (2014-08-11 15:56:06 UTC) #21
rsc
10 years, 9 months ago (2014-08-11 17:46:29 UTC) #22
Thank you for that link. I filed Apple Bug 17978656.

Russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b