Bug 15922

I have a feeling this has to do with the non-caching userdb code path.  Please look at bug #15588 and compile with caching on to see if this issue is fixed.  For reference bug #15589 has the noncaching issue.  Basically userinfo structures need to become objects with references. 

I recompiled with --enable-userdb-cache but that did not solve the problem. :-(

did you apply the patch also? If not then that flag won't actually do anythin.  That was part of the bug.

Yes, I applied that patch :-(

is there anything I can do to help finding the problem here?

Having a look at valgrind's
--snip--
Thread 1: status = VgTs_Runnable
==3603==    at 0x4822B8E: realloc (vg_replace_malloc.c:429)
==3603==    by 0x32641: dbus_realloc (dbus-memory.c:601)
==3603==    by 0x32E69: reallocate_for_length (dbus-string.c:364)
==3603==    by 0x32F14: set_length (dbus-string.c:404)
==3603==    by 0x332B2: _dbus_string_lengthen (dbus-string.c:872)
==3603==    by 0x334DC: append (dbus-string.c:1020)
==3603==    by 0x33572: _dbus_string_append (dbus-string.c:1050)
==3603==    by 0x3C8B2: _dbus_directory_get_next_file (dbus-sysdeps-util-unix.c:761)
==3603==    by 0x3E63: update_directory (activation.c:577)
==3603==    by 0x41E0: bus_activation_new (activation.c:771)
==3603==    by 0x6602: process_config_every_time (bus.c:497)
==3603==    by 0x68E2: bus_context_new (bus.c:615)
==3603==    by 0x1B2A1: main (main.c:443)
--snap--

I did a 

--snip--

$ gdb /usr/bin/dbus-daemon 
GNU gdb 6.7.1-debian
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i486-linux-gnu"...
Using host libthread_db library "/lib/libthread_db.so.1".
(gdb) break _dbus_directory_open
Breakpoint 1 at 0x3c5ee: file dbus-sysdeps-util-unix.c, line 637.
(gdb) break _dbus_directory_get_next_file
Breakpoint 2 at 0x3c756: file dbus-sysdeps-util-unix.c, line 727.
(gdb) run --session --nofork
Starting program: /usr/bin/dbus-daemon --session --nofork
Breakpoint 1 at 0x3c5db: file dbus-sysdeps-util-unix.c, line 630.
Breakpoint 2 at 0x3c743: file dbus-sysdeps-util-unix.c, line 720.
Warning:
Cannot insert breakpoint 1.
Error accessing memory address 0x3c5db: Input/output error.
Cannot insert breakpoint 2.
Error accessing memory address 0x3c743: Input/output error.

--snap--

Any idea what that is?

Created attachment 16746 [details] [review]
changes readdir_r buffer for fuse file system (and maybe others)

With my latest attachment / patch, dbus works for me with the root file system on fuse. 
It was a bit odd to track down - it turned out that in the calculation of the buffer for readdir_r the maximum length for a filename had a value of 0.

hmm. any opinion on the patch and/or the problem?
Feedback appreciated.

I can confirm this problem and that the patch fixes it.

FWIW, *my* only FUSE filesystem is mounted at /usr

This code requires some care:
http://womble.decadentplace.org.uk/readdir_r-advisory.html

There is no indication in the standard that fpathconf should ever be returning 0:
http://www.opengroup.org/onlinepubs/009695399/functions/pathconf.html

Surely this is a bug in fuse.

Not sure falling back to PATH_MAX is safe. In fact I think it isn't. readdir_r will smash the stack if the filename is longer than the buffer...

Falling back to 0 is certainly unsafe (*any* filename will overflow it and smash the stack), so I don't think we can really make the situation any worse by replacing 0 with NAME_MAX... I'm very tempted to just make the buffer size be NAME_MAX if fpathconf() returns less than that. It can't hurt.

For comparison, GLib's g_dir_read_name() calls readdir() (without holding a lock, which doesn't seem ideal). An alternative approach would be to take a lock around readdir().

I've asked Ben (author of that readdir_r advisory) for input.

For further comparison, Qt calls pathconf, with a comment explaining the race condition, and doesn't handle pathconf returning 0 either.

(In reply to comment #13)
> For further comparison, Qt calls pathconf, with a comment explaining the race
> condition, and doesn't handle pathconf returning 0 either.

Because, like Havoc said, pathconf is not allowed to return 0 unless the limit is *really* a zero, meaning zero bytes.

Created attachment 41708 [details] [review]
[PATCH 1/3] dirent_buf_size: always assume readdir_r needs at least 255 bytes

HP-UX apparently defines NAME_MAX too small. Sigh.

Created attachment 41709 [details] [review]
[PATCH 2/3] dirent_buf_size: always assume that readdir_r needs at least NAME_MAX bytes

fpathconf() can apparently return 0 for the max length of a filename,
at least on zfs-on-fuse. Sigh.

(This patch hopefully fixes the bug stated here.)

Created attachment 41710 [details] [review]
[PATCH 3/3] dirent_buf_size: always demand at least sizeof(struct dirent) bytes

As per http://womble.decadent.org.uk/readdir_r-advisory.html rev 4,
d_name might not be the last member in struct dirent.

(In reply to comment #14)
> Because, like Havoc said, pathconf is not allowed to return 0 unless the limit
> is *really* a zero, meaning zero bytes.

As far as I can tell, glibc on Linux basically returns f_namelen from struct statfs, which is a copy of f_namemax from struct statvfs, so if a filesystem fails to set that, we lose. Being a bit defensive by allocating more memory than we strictly needed to seems like a better user experience than smashing our own stack in protest at a filesystem bug, particularly when userland filesystems exist.

On Bug #8284, Havoc points out that we don't really need thread-safety at all, because we only call readdir_r() from the bus daemon and the tests; so we could probably just use readdir() and bypass this whole mess.

(In reply to comment #19)
> On Bug #8284, Havoc points out that we don't really need thread-safety at all,
> because we only call readdir_r() from the bus daemon and the tests; so we could
> probably just use readdir() and bypass this whole mess.

On Linux/glibc readdir() is threadsafe already, because the name is stored in DIR, not in struct dirent, which really, is the only sane thing to do.

It's not reentrant, but libdbus isn't giving out its DIR * pointers, so it doesn't matter.

Fixed in git for 1.4.4, 1.5.0 by merging the patch from Bug #8284, so we no longer use readdir_r at all. Thanks to Will Thompson for code review.