libfs dcache_readdir() and dcache_dir_lseek() bugfix

This patch effects all users of libfs' dcache directory implementation.

Old glibc implementations (e.g. glibc-2.2.5) are lseeking after every call to getdents(), subsequent calls to getdents() are starting to read from a wrong f_pos, when the directory is modified in between. Therefore not all directory entries are returned. IMHO this is a bug and it breaks applications, e.g. "rm -fr" on tmpfs.

SuSV3 only says:
"If a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir_r() returns an entry for that file is unspecified."

Quoted 5 lines

SuSV3 only says:
"If a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir_r() returns an entry for that file is unspecified."

IOW, the applications in question are broken since they rely on unspecified behaviour, not provided by old libc versions.

Quoted 6 lines

Quoted 5 lines

SuSV3 only says: "If a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir_r() returns an entry for that file is unspecified."

Quoted 2 lines

IOW, the applications in question are broken since they rely on unspecified behaviour, not provided by old libc versions.

No. SuSV3 only says that the behavior of readdir() is unspecified w.r.t. an entry for the removed/added file. I think readdir() should still return the entries which are not removed/added. What do you think?

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

SuSV3 only says: "If a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir_r() returns an entry for that file is unspecified."

IOW, the applications in question are broken since they rely on unspecified behaviour, not provided by old libc versions.

Quoted 4 lines

No. SuSV3 only says that the behavior of readdir() is unspecified w.r.t. an entry for the removed/added file. I think readdir() should still return the entries which are not removed/added. What do you think?

What 'rm' is this? Mine (coreutils 5.2.1) doesn't do any seeking and I don't think that glibc does either.

As I said: "Old glibc implementations (e.g. glibc-2.2.5) are lseeking after every call to getdents() ..."

Precisely this is a SLES8 on s390-64bit.

s390vm02:/# rpm -qf /bin/rm
fileutils-4.1.11-144
s390vm02:/# rpm -q glibc
glibc-2.2.5-234

But you can also try my testcase.

Quoted 2 lines

As I said: "Old glibc implementations (e.g. glibc-2.2.5) are lseeking after every call to getdents() ..."

Hmm, why would it do that? This seems like it's glibc being stupid.

That said, you are right that the libfs readdir implementation is not strictly standards conforming. But neither is your patch: this algorithm will break if the entry at the current position is removed and then a new entry with the same name is created.

Unfortunately I can't since I don't have such old glibc.

True. Seeking to that offset should at least fail and shouldn't stop at the new entry. But SuSV3 says that the offset given by telldir() is valid until the next rewinddir(). This is no problem for directories that can only grow. I tried to implement some kind of deferred dput'ing of the d_child's but that was too hackish and was wasting memory. So the best thing I can do now is fail if someone wants to seek to an offset of an already unlinked file.

Quoted 7 lines · On Fri, 4 November 2005 16:11:04 +0100, [email protected] wrote:

True. Seeking to that offset should at least fail and shouldn't stop at the new entry. But SuSV3 says that the offset given by telldir() is valid until the next rewinddir(). This is no problem for directories that can only grow. I tried to implement some kind of deferred dput'ing of the d_child's but that was too hackish and was wasting memory. So the best thing I can do now is fail if someone wants to seek to an offset of an already unlinked file.

Does that mean that, to satisfy the standard, you'd have to allow the seek, but return 0 bytes on further reads, as you're already at (or beyond, whatever) EOF?

Quoted 9 lines · Jörn Engel

Quoted 7 lines · On Fri, 4 November 2005 16:11:04 +0100, [email protected] wrote:

True. Seeking to that offset should at least fail and shouldn't stop at the new entry. But SuSV3 says that the offset given by telldir() is valid until the next rewinddir(). This is no problem for directories that can only grow. I tried to implement some kind of deferred dput'ing of the d_child's but that was too hackish and was wasting memory. So the best thing I can do now is fail if someone wants to seek to an offset of an already unlinked file.

Quoted 3 lines

Does that mean that, to satisfy the standard, you'd have to allow the seek, but return 0 bytes on further reads, as you're already at (or beyond, whatever) EOF?

No. To satisfy the standard, it would be necessary to let the seek succeed and to return the already unlinked dentry or the next dentry (this is unspecified).

I think we should return the next dentry, therefore I let the seek fail (seekdir() doesn't even have a return value) and the cursor/f_pos is still at the old offset.

The real problem is this IMHO:
...
telldir() = a
...
telldir() = b
readdir() = foo.txt
unlink(foo.txt)
seekdir(a)
seekdir(b)
readdir() = ???

With my patch the seekdir(b) doesn't find the offset and is placing the cursor at the end of the directory. In my understanding of the SuSV3 this should be possible and should return either "foo.txt" or the next entry after "foo.txt". I don't see any chance how I can implement that.

Quoted 8 lines

True. Seeking to that offset should at least fail and shouldn't stop at the new entry. But SuSV3 says that the offset given by telldir() is valid until the next rewinddir(). This is no problem for directories that can only grow. I tried to implement some kind of deferred dput'ing of the d_child's but that was too hackish and was wasting memory. So the best thing I can do now is fail if someone wants to seek to an offset of an already unlinked file.

Does that mean that, to satisfy the standard, you'd have to allow the seek, but return 0 bytes on further reads, as you're already at (or beyond, whatever) EOF?

Sounds quite ugly and it would be nice if this wasn't necessary.

Quoted 5 lines

Quoted 2 lines

As I said: "Old glibc implementations (e.g. glibc-2.2.5) are lseeking after every call to getdents() ..."

Hmm, why would it do that? This seems like it's glibc being stupid.

Well, glibc is that stupid and triggers the bug.

Quoted 4 lines

That said, you are right that the libfs readdir implementation is not strictly standards conforming. But neither is your patch: this algorithm will break if the entry at the current position is removed and then a new entry with the same name is created.

True. Seeking to that offset should at least fail and shouldn't stop at the new entry.

But SuSV3 says that the offset given by telldir() is valid until the next rewinddir(). This is no problem for directories that can only grow. I tried to implement some kind of deferred dput'ing of the d_child's but that was too hackish and was wasting memory. So the best thing I can do now is fail if someone wants to seek to an offset of an already unlinked file.

So I can include the inode number in the hashing process somehow. Any ideas on that one?

Quoted 2 lines

Unfortunately I can't since I don't have such old glibc.

The testcase is similar to what "rm *" with the old glibc would do. It just a testcase to show where the problem is.

Quoted 2 lines

Well, glibc is that stupid and triggers the bug.

Seems to me, the simple solution is to upgrade your glibc.

Quoted 3 lines

True. Seeking to that offset should at least fail and shouldn't stop at the new entry.

No it should _not_ fail, it should continue from the next _existing_ entry.

No good. Same problem if you move out then move back the entry.

Quoted 5 lines

Quoted 2 lines

Well, glibc is that stupid and triggers the bug.

Seems to me, the simple solution is to upgrade your glibc.

This is SLES8. You don't want to update the glibc.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Well, glibc is that stupid and triggers the bug.

Seems to me, the simple solution is to upgrade your glibc.

Quoted 1 line

This is SLES8. You don't want to update the glibc.

OK, but then it's basically a SLES8 kernel issue, not a mainline kernel issue.

Probably very few people are using brand new kernels with glibc from the last millennium ;)

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Well, glibc is that stupid and triggers the bug.

Seems to me, the simple solution is to upgrade your glibc.

Quoted 8 lines

Quoted 1 line

This is SLES8. You don't want to update the glibc.

OK, but then it's basically a SLES8 kernel issue, not a mainline kernel issue.

Probably very few people are using brand new kernels with glibc from the last millennium ;)

Hmm, so I should only send patches for bugs that are often triggered upstream? Just start using seekdir() and modify the directory contents on an upstream tmpfs. You'll see that this isn't working well.

This is a bug and it should get fixed. I hope that it doesn't depend on how many people using what library :)

Your patch is not a solution, since readdir will remain nonconforming. It is basically a workaround for a bug in glibc.

It makes readdir
nonconforming in a different way, but the end result in not necessarily better.

If you manage to make dcache_readdir conform to SUS without overly bloating the implementation, that's fine.

This is a bug and it should get fixed.

Quoted 5 lines · Miklos Szeredi

Quoted 2 lines

As I said: "Old glibc implementations (e.g. glibc-2.2.5) are lseeking after every call to getdents() ..."

Hmm, why would it do that? This seems like it's glibc being stupid.

Well, glibc is that stupid and triggers the bug.

Quoted 1 line

Unfortunately I can't since I don't have such old glibc.

The testcase is similar to what "rm *" with the old glibc would do. It just a testcase to show where the problem is.

The new bug is rather that glibc will return EOVERFLOW, and try to rewind your file pointer if your filesystem happens to return 64-bit offsets to getdents64().

Quoted 8 lines · On Fri, Nov 04, 2005 at 12:38:51PM +0100, [email protected] wrote:

This patch effects all users of libfs' dcache directory implementation.

Old glibc implementations (e.g. glibc-2.2.5) are lseeking after every call to getdents(), subsequent calls to getdents() are starting to read from a wrong f_pos, when the directory is modified in between. Therefore not all directory entries are returned. IMHO this is a bug and it breaks applications, e.g. "rm -fr" on tmpfs.

Quoted 4 lines

SuSV3 only says:
"If a file is removed from or added to the directory after the most recent call to opendir() or rewinddir(), whether a subsequent call to readdir_r() returns an entry for that file is unspecified."

IOW, the applications in question are broken since they rely on unspecified behaviour, not provided by old libc versions.

That said, I'm pretty sure it's the old libc behavior that's defective. If a

#include <stdio.h>
#include <unistd.h>
#include <dirent.h>

int main(int argc, char *argv[])

{
	DIR *dir;
	struct dirent *entry;
	unsigned int offset;

        if (argc < 2) {
                printf("USAGE:  %s <directory>\n", argv[0]);
                return 1;
        }

	dir = opendir(argv[1]);
	if (!dir)

		return 1;

	while((entry = readdir(dir)) != NULL) {
		seekdir(dir, entry->d_off);
		printf("name=%s\tino=%d off=%d\n", entry->d_name, entry->d_ino,
		       entry->d_off);
		if (*entry->d_name == '.')
			continue;

		if(unlink(entry->d_name) != 0)
			break;
	}

	closedir(dir);
	return 0;
}