Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getdents64 misses last 100 entries? #2077

Closed
redbaron opened this issue May 8, 2017 · 12 comments
Closed

getdents64 misses last 100 entries? #2077

redbaron opened this issue May 8, 2017 · 12 comments

Comments

@redbaron
Copy link

redbaron commented May 8, 2017

I am running Windows 10 1703 build 16215.1000 and it seems that getdents64 doesn't return all the entries, more specifically it seems to miss last 100 entries.

Self contained script to reproduce a problem follows. I build it while debugging Go project compilation issue, so it might not be an absolute minimal test case, but it reproduces error 100% of the time, so it might be a good start:

#!/bin/bash
set -ue

cat >test.go <<"EOF"

package main

import (
   "os"
   "fmt"
)

func main() {
   if err := os.RemoveAll("testdir"); err != nil {
        fmt.Printf("ERROR:%s\n", err)
   }
}
EOF


[[ -d go ]] || {
   echo "Downloading and unpacking go 1.8.1"
   curl -L -s https://storage.googleapis.com/golang/go1.8.1.linux-amd64.tar.gz | tar -xz
}

GO=./go/bin/go
export GOROOT=./go

go build test.go

rm -rf testdir
git clone https://go.googlesource.com/sys testdir
(cd testdir; git checkout 9ccfe848b9db8435a24c424abbc07a921adf1df5)

strace -s 1000 -o strace.log -f ./test || echo "os.RemoveAll() failed, trying again"
#./test
#echo "Works second time"

run it in empty dir on a lxfs mount (I didn't check drvfs yet).

You'll see that it can't delete directory testdir/unix with an error

ERROR:remove testdir/unix: directory not empty

looking at what is left in testdir/unix, you'll see that exactly 100 files are left there. Script produces strace.log output with following lines among others:

10533 unlinkat(AT_FDCWD, "testdir/unix", AT_REMOVEDIR) = -1 ENOTEMPTY (Directory not empty)
10533 lstat("testdir/unix", {st_mode=S_IFDIR|0755, st_size=512, ...}) = 0
10533 openat(AT_FDCWD, "testdir/unix", O_RDONLY|O_CLOEXEC) = 4
10533 getdents64(4, /* 110 entries */, 4096) = 4070

so it returns 110 entries, but on a clean git clone at a given commit, testdir/unix directory has 210 entries

It is not reproducible with rm -rf though, probably because coreutils use getdents version of syscall, which seem to work fine

@therealkenc
Copy link
Collaborator

therealkenc commented May 8, 2017

The second time getdents64(4, ...) gets called something goes awry. strace interprets the buffer as containing 3 entries. Golang finds more, but not 100 more (which wouldn't fit in 127 bytes anyway). Possibly something was disturbed by #1769. Possibly this exact pattern hasn't tripped before. Full trace is here.

<...first call to getdents64() with 110 entries per OP>
[pid  1172] openat(AT_FDCWD, "testdir/unix", O_RDONLY|O_CLOEXEC) = 4
[pid  1172] getdents64(4, /* 110 entries */, 4096) = 4070
<...here is the next call to getdents64() on fd 4>
[pid  1172] getdents64(4, /* 3 entries */, 4096) = 127
[pid  1172] getdents64(4, /* 0 entries */, 4096) = 0
[pid  1172] unlinkat(AT_FDCWD, "testdir/unix/syscall_unix_gc.go", 0) = 0
[pid  1172] unlinkat(AT_FDCWD, "testdir/unix/syscall_unix_test.go", 0) = 0
[pid  1172] getdents64(4, /* 3 entries */, 4096) = 127
[pid  1172] getdents64(4, /* 0 entries */, 4096) = 0
<...more than 3 get subsequently unlinked but not 100>
[pid  1172] unlinkat(AT_FDCWD, "testdir/unix/types_darwin.go", 0) = 0
[pid  1172] unlinkat(AT_FDCWD, "testdir/unix/types_dragonfly.go", 0) = 0
<...>
[pid  1172] unlinkat(AT_FDCWD, "testdir/unix/ztypes_openbsd_386.go", 0) = 0
[pid  1172] unlinkat(AT_FDCWD, "testdir/unix/ztypes_openbsd_amd64.go", 0) = 0
[pid  1172] unlinkat(AT_FDCWD, "testdir/unix/ztypes_solaris_amd64.go", 0) = 0
[pid  1172] getdents64(4, /* 0 entries */, 4096) = 0
[pid  1172] close(4)                    = 0

@redbaron
Copy link
Author

redbaron commented May 9, 2017

@therealkenc , shouldn't second call just return 0 (EOF)? First call passes buffer 4096 entries long, surely whole response would fit there

@therealkenc
Copy link
Collaborator

therealkenc commented May 9, 2017

Here is the same test on Real Linux™. The first call coughs up 94 entries.

[pid 30810] unlinkat(AT_FDCWD, "testdir/unix", 0) = -1 EISDIR (Is a directory)
[pid 30810] unlinkat(AT_FDCWD, "testdir/unix", AT_REMOVEDIR) = -1 ENOTEMPTY (Directory not empty)
[pid 30810] lstat("testdir/unix", {st_mode=S_IFDIR|0777, st_size=12288, ...}) = 0
[pid 30810] openat(AT_FDCWD, "testdir/unix", O_RDONLY|O_CLOEXEC) = 4
[pid 30810] getdents64(4, /* 94 entries */, 4096) = 4088
[...]

The ABI doesn't make any guarantees about how the entries are packed, only that d_off points to the next entry.

@redbaron
Copy link
Author

redbaron commented Jun 9, 2017

@benhillis , @zadjii-msft this was still not labeled as a bug, which is concerning, were you able to reproduce it on your systems?

@redbaron
Copy link
Author

redbaron commented Jun 9, 2017

Just checked Build 16215, problem still present

@zadjii-msft zadjii-msft added the bug label Jun 9, 2017
@zadjii-msft
Copy link
Member

I'll mark it as a bug so that the kernel team can triage.

@sunilmut
Copy link
Member

sunilmut commented Jun 9, 2017

Also adding @SvenGroot as FYI.

@SvenGroot
Copy link
Member

It turns that that we can end up skipping entries if you unlink files in between calls to getdents(64). I'm working on fixes for both LxFs and DrvFs.

@benhillis
Copy link
Member

Fixed in 16237.

@redbaron
Copy link
Author

I can confirm that it is fixed now.

@izbyshev
Copy link

I've encountered a variant of this issue in 1809 (Ubuntu 18.04), so apparently this is not (completely) fixed. When reading /proc/self/fd, if there are several getdents[64] syscalls and we close some of the descriptors corresponding to the entries we've got after each syscall, we'll stop early.

$ cat test.c
#define _GNU_SOURCE
#include <fcntl.h>
#include <sys/syscall.h>
#include <unistd.h>

int main(void) {
    char buf[100];
    for (int i = 0; i < 10; i++)
        open("/dev/null", O_RDONLY);
    int dfd = open("/proc/self/fd", O_RDONLY);
    int fd = 4;
    while (syscall(SYS_getdents64, dfd, buf, sizeof(buf)) > 0)
        close(fd++);
    close(dfd);
    return 0;
}
$ gcc test.c
$ strace -e openat,close,getdents64 ./a.out
[...]
openat(AT_FDCWD, "/proc/self/fd", O_RDONLY) = 13
getdents64(13, /* 4 entries */, 100)    = 96
close(4)                                = 0
getdents64(13, /* 4 entries */, 100)    = 96
close(5)                                = 0
getdents64(13, /* 4 entries */, 100)    = 96
close(6)                                = 0
getdents64(13, /* 1 entries */, 100)    = 24
close(7)                                = 0
getdents64(13, /* 0 entries */, 100)    = 0
close(13)                               = 0
[...]

The expected strace output is:

openat(AT_FDCWD, "/proc/self/fd", O_RDONLY) = 13
getdents64(13, /* 4 entries */, 100)    = 96
close(4)                                = 0
getdents64(13, /* 4 entries */, 100)    = 96
close(5)                                = 0
getdents64(13, /* 4 entries */, 100)    = 96
close(6)                                = 0
getdents64(13, /* 3 entries */, 100)    = 72
close(7)                                = 0
getdents64(13, /* 0 entries */, 100)    = 0
close(13)                               = 0

The example was reduced from subprocess Python module. Its test suite fails because subprocess uses similar logic to close open descriptors in the child process.

getdents syscall is also affected.

@therealkenc
Copy link
Collaborator

Spawn into a new issue. Nice repro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants