|
Description
Simon McVittie
2013-08-27 13:35:07 UTC
Comment on attachment 84714 [details] [review] _dbus_get_tmpdir: be thread-safe Oops, that patch doesn't actually compile (a variable is undeclared). I'll try again. Created attachment 84715 [details] [review] _dbus_get_tmpdir: be thread-safe Sharing a static variable between threads is not safe in general, and this function is used in the shared libdbus (for nonce files), so it can't rely on being single-threaded. Comment on attachment 84715 [details] [review] _dbus_get_tmpdir: be thread-safe Review of attachment 84715 [details] [review]: ----------------------------------------------------------------- looks good There are a bunch of other misused static variables. I'll attach easy patches for most of them, but the Windows implementations of _dbus_replace_install_prefix() and _dbus_windows_get_datadir() can't be thread-safe without a redesign: sharing a static buffer for different results just can't work. I'd suggest making them append to a DBusString - then the one caller of _dbus_replace_install_prefix() outside the sysdeps, which currently uses _dbus_strdup() on the result (without checking for NULL!), can be changed to use _dbus_string_steal_data(). Created attachment 84830 [details] [review] _dbus_file_path_extract_elements_from_tail: don't misuse static variable If we _dbus_verbose() from more than one thread at the same time, we don't want to get into trouble with static variables (and I don't think micro-optimizing this function is really worth it anyway). Created attachment 84831 [details] [review] _dbus_get_autolaunch_address: don't make argv static This function could be accessed from any thread, which would mean it scribbles on argv twice. Created attachment 84832 [details] [review] Comment some suspicious uses of static variables Created attachment 84833 [details] [review] _dbus_win_startup_winsock: be thread-safe Created attachment 84834 [details] [review] _dbus_check_setuid: comment on thread-safety Created attachment 84837 [details] [review] Move some sysdeps stuff only used by the dbus-daemon outside libdbus This means we don't need to worry about whether it's thread-safe, and makes libdbus a little smaller. --- (In reply to comment #4) > the Windows implementations > of _dbus_replace_install_prefix() and _dbus_windows_get_datadir() can't be > thread-safe without a redesign: sharing a static buffer for different > results just can't work. They're still not thread-safe, but now it doesn't matter. Comment on attachment 84830 [details] [review] _dbus_file_path_extract_elements_from_tail: don't misuse static variable Review of attachment 84830 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 84831 [details] [review] _dbus_get_autolaunch_address: don't make argv static Review of attachment 84831 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 84832 [details] [review] Comment some suspicious uses of static variables Review of attachment 84832 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 84833 [details] [review] _dbus_win_startup_winsock: be thread-safe Review of attachment 84833 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 84834 [details] [review] _dbus_check_setuid: comment on thread-safety Review of attachment 84834 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +4177,2 @@ > static dbus_bool_t check_setuid_initialised; > static dbus_bool_t is_setuid; no need to set an initial value on link time ? Comment on attachment 84837 [details] [review] Move some sysdeps stuff only used by the dbus-daemon outside libdbus Review of attachment 84837 [details] [review]: ----------------------------------------------------------------- comiled and cross compiled with cmake on linux without any problems looks good (In reply to comment #15) > no need to set an initial value on link time ? No, C guarantees that extern and static variables are zeroed if not otherwise initialized (whereas "auto" variables - i.e. local variables on the stack - have undefined contents). On Unix this is usually implemented by the runtime linker: the sections in a binary include "initialized variables" (with the initial values stored in the binary, and mapped with copy-on-write) and "uninitialized variables" (with only a length in the binary, and the linker allocates enough writable, initially-zero memory to match the size of that section). (In reply to comment #17) > No, C guarantees that extern and static variables are zeroed if not > otherwise initialized (Strictly speaking, it guarantees that arithmetic types are 0 and pointers are NULL, but those correspond to all-bits-zero on any sensible platform.) Created attachment 85080 [details] [review] Fix declaration of _dbus_win_startup_winsock This regressed in commit b7a91bfd. --- ... which was Attachment #84833 [details]. Sorry about that. Comment on attachment 85080 [details] [review] Fix declaration of _dbus_win_startup_winsock Review of attachment 85080 [details] [review]: ----------------------------------------------------------------- looks good (In reply to comment #20) > Fix declaration of _dbus_win_startup_winsock > looks good Thanks, merged. Created attachment 85113 [details] [review] _dbus_modify_sigpipe: be thread-safe This needs new atomic primitives: we don't have "set to a value", and in fact that's a bit annoying to implement in terms of gcc intrinsics. "Set to 0" and "set to nonzero" are easy, though. Also add a trivial sanity-check for the atomic primitives (it doesn't verify that they're atomic, but does verify that they return the right things). Remaining static things whose thread-safety looks suspicious: * dbus-internals.c: verbose_initted, verbose (not particularly important) * dbus-internals.c: module_name (Windows only) * dbus-sysdeps-thread-win.c: dbus_cond_event_tls (Windows only) * dbus-sysdeps-win.c: hDBusDaemonMutex etc. (Windows only) Created attachment 85241 [details] [review] various: comment static variables that are locked or otherwise OK -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/88. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.