Created attachment 43657 [details] [review] DBusNonceFile: don't always fail when use_subdir=TRUE, i.e. on Unix While writing a simple black-box test for libdbus, I thought I'd try out all the possible transports. It turns out the nonce-tcp transport has never worked on non-Windows. The Unix code is meant to create a file _dbus_get_tmpdir() + "/dbus-XXXXX/nonce" (where XXXXX is random), creating the intermediate directory first. Due to a typo, it tries to mkdir the whole path (including the /nonce); this fails, because the intermediate directory doesn't exist yet. The Windows code blindly assumes that _dbus_get_tmpdir() is private and owned by the user, and creates a file _dbus_get_tmpdir() + "/dbus-XXXXX". This works, but is pointlessly divergent (if we used the Unix code path in both cases, we'd have found the bug sooner!), and following the Unix code path would protect us against the _dbus_get_tmpdir() returning C:\TEMP or something. I suspect the idea of making the intermediate directory is to avoid symlink attacks. It may just be cargo-cult programming - we do use O_EXCL on Unix (and CREATE_NEW on Windows) - but if it's meant to be a defence against symlink attacks or whatever, there's no point in using mkdir() unless we treat EEXISTS as an error (which would mean we should introduce a "dbus_bool_t ok_if_exists" parameter to _dbus_create_directory).
Created attachment 44313 [details] [review] regression test This requires the "modular tests" infrastructure from Bug #34570; it's the same as the test previously attached there.
If people don't want Bug #34570 (integration tests) in 1.4, we could merge this in 1.5, and cherry-pick only the actual fix for 1.4.
Interestingly, GDBus supports nonce-tcp on Unix too. FWIW, I never tested interop with libdbus on Unix, only on Win32. Let me check your patch with the GDBus implementation...
(In reply to comment #3) > Interestingly, GDBus supports nonce-tcp on Unix too. FWIW, I never tested > interop with libdbus on Unix, only on Win32. Let me check your patch with the > GDBus implementation... OK, seems to work fine with both libdbus-1 and GDBus (except for some debug spew but I don't think that's important): [davidz@x61 bus]$ ./dbus-daemon --session --address=nonce-tcp: --print-address nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802 ^Z [2]+ Stopped ./dbus-daemon --session --address=nonce-tcp: --print-address [davidz@x61 bus]$ bg [2]+ ./dbus-daemon --session --address=nonce-tcp: --print-address & [davidz@x61 bus]$ DBUS_SESSION_BUS_ADDRESS="nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802" ../tools/dbus-send --session --print-reply --dest=org.freedesktop.DBus / org.freedesktop.DBus.GetId Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid method return sender=org.freedesktop.DBus -> dest=:1.0 reply_serial=2 string "5dacb1acc49945cb1774243100004802" [davidz@x61 bus]$ DBUS_SESSION_BUS_ADDRESS="nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802" gdbus call --session --dest org.freedesktop.DBus --object-path / --method org.freedesktop.DBus.GetId ('5dacb1acc49945cb1774243100004802',)
Off-topic, yet maybe still interesting: this also demonstrates that DBUS_COOKIE_SHA1 interop works (normally EXTERNAL with credentials is used): $ G_DBUS_DEBUG=authentication DBUS_SESSION_BUS_ADDRESS="nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802" gdbus call --session --dest org.freedesktop.DBus --object-path / --method org.freedesktop.DBus.GetId GDBus-debug:Auth: CLIENT: initiating GDBus-debug:Auth: CLIENT: didn't send any credentials GDBus-debug:Auth: CLIENT: writing `AUTH\r\n' GDBus-debug:Auth: CLIENT: WaitingForReject GDBus-debug:Auth: CLIENT: WaitingForReject, read 'REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS' GDBus-debug:Auth: CLIENT: Trying to choose mechanism GDBus-debug:Auth: CLIENT: Trying mechanism `EXTERNAL' GDBus-debug:Auth: CLIENT: Mechanism `EXTERNAL' says it is not supported GDBus-debug:Auth: CLIENT: Trying to choose mechanism GDBus-debug:Auth: CLIENT: Trying mechanism `DBUS_COOKIE_SHA1' GDBus-debug:Auth: CLIENT: writing `AUTH DBUS_COOKIE_SHA1 353030\r\n' GDBus-debug:Auth: CLIENT: WaitingForData GDBus-debug:Auth: CLIENT: WaitingForData, read=`DATA 6f72675f667265656465736b746f705f67656e6572616c20353734373432363730203837313933343837613738373062333665613131613032353864303239316638' GDBus-debug:Auth: CLIENT: writing `DATA 487962796941436b544868646c7266612031666231626339626264643864373861646637626635376633396266653038363431353661666665\r\n' GDBus-debug:Auth: CLIENT: WaitingForOK GDBus-debug:Auth: CLIENT: WaitingForOK, read `OK 7de11038e49f80c15d7695e000004802' GDBus-debug:Auth: CLIENT: writing `BEGIN\r\n' GDBus-debug:Auth: CLIENT: Done, authenticated=1 ('5dacb1acc49945cb1774243100004802',)
(In reply to comment #0) > Created an attachment (id=43657) [details] > DBusNonceFile: don't always fail when use_subdir=TRUE, i.e. on Unix > Feel free to add Tested-by: David Zeuthen <davidz@redhat.com> to the commit message.
(In reply to comment #4) > (In reply to comment #3) > OK, seems to work fine with both libdbus-1 and GDBus (except for some debug > spew but I don't think that's important): Did you review the one-line patch? May I merge it? :-) (Review of the regression test would also be appreciated; it requires Bug #34570.)
(In reply to comment #7) > (In reply to comment #4) > > (In reply to comment #3) > > OK, seems to work fine with both libdbus-1 and GDBus (except for some debug > > spew but I don't think that's important): > > Did you review the one-line patch? May I merge it? :-) Yup, I looked at the patch too. Looks good to merge... Feel free to also add Reviewed-by: davidz tag in addition to the Tested-by: davidz tag :-) (Is there a flag I should have toggled to indicate this but didn't? I mean, if this was b.g.o I'd set the patch/attachment status to 'reviewed' or, in case I was the maintainer, 'accepted-commit-now'.) > (Review of the regression test would also be appreciated; it requires Bug > #34570.) Sure, let me take a look.
(In reply to comment #1) > Created an attachment (id=44313) [details] > regression test > > This requires the "modular tests" infrastructure from Bug #34570; it's the same > as the test previously attached there. This looks good to me. (Though, this could fail if there's an extremely prohibitive firewall that doesn't allow listening/connecting on the loopback interface (think distro builder where you cannot trust the source / build scripts) but I think the answer here is then "don't run the D-Bus test suite, then".) Reviewed-by: David Zeuthen <davidz@redhat.com>
Thanks for reviewing! (In reply to comment #8) > (Is there a flag I should have toggled to indicate this but didn't? I mean, if > this was b.g.o I'd set the patch/attachment status to 'reviewed' or, in case I > was the maintainer, 'accepted-commit-now'.) Any comment that indicates that you've reviewed it, really ("ship it" or "review+" or something). In Telepathy we often put review+ in the "Status Whiteboard" so it can show up in bug lists (at least for people like me who have the Status Whiteboard in their configured column-list).
(In reply to comment #8) > or, in case I was the maintainer Oh, you're not in the approved-reviewer list in HACKING, are you? :-( Perhaps you should be...
(In reply to comment #11) > (In reply to comment #8) > > or, in case I was the maintainer > > Oh, you're not in the approved-reviewer list in HACKING, are you? :-( Perhaps > you should be... I don't think I am, no.
Now that davidz is in the reviewer cabal: fixed in git for 1.4.10, will be merged to master shortly (for 1.5.2).
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.