libdbus has historically supported two semi-cross-platform credentials-passing schemes on NetBSD: * LOCAL_CREDS was recently removed from master (Bug #60340) because it regressed in 2009 and no longer compiled. It can only pass through the uid (and the gid, which we don't use), and requires assistance from the peer. Also, when I got it to compile again, it didn't actually *work* on NetBSD, probably for the reasons described in <http://julipedia.meroh.net/2006/08/localcreds-socket-credentials.html>. * getpeereid() still works, and doesn't require assistance from the peer, but again, it can only pass through the uid and gid. NetBSD also has its own OS-specific thing, LOCAL_PEEREID, which is rather like Linux/OpenBSD SO_PEERCRED: it *can* pass the pid through, and does not require assistance from the peer. That makes it perfect for D-Bus. At the moment, the NetBSD pkgsrc "port" of libdbus patches in LOCAL_PEEREID support as part of an apparently unrelated patch titled "_dbus_poll: Set the timeout value argument to poll to -1 whenever it is less than -1 to avoid kde4 session start hang". Now that LOCAL_CREDS has gone away, this patch should get a lot simpler. I would like libdbus distributors to contribute their various portability patches upstream, so that they can be reviewed by libdbus maintainers, rather than being downstream divergence forever. Unfortunately, when I tried implementing LOCAL_PEEREID, I didn't seem to be receiving the pid (GetConnectionUnixProcessID() raises org.freedesktop.DBus.Error.UnixProcessIdUnknown), which suggests that something went wrong in my implementation. getpeereid() is "good enough" so I'm not going to spend much time on this myself, but I'd be happy to review tested patches.
Attachment #85791 [details] is as far as I got. NetBSD developers are invited to make it work :-) +#ifdef __NetBSD__ + /* Not included in AC_USE_SYSTEM_EXTENSIONS, but similar. */ +# define _NETBSD_SOURCE +#endif This part might not be necessary. If it isn't, I'd prefer not to have it.
Created attachment 97249 [details] [review] NetBSD credential passing I should have checked before writing this - it is practically identical to your patch. I'd be happy to test yours if you prefer. Test: - dbus-1.8.0 on NetBSD-6.99.40/amd64 - run at-spi2-registryd - execute: dbus-send --print-reply --dest=org.freedesktop.DBus /org/freedesktop/DBus \ org.freedesktop.DBus.GetConnectionUnixProcessID \ string:'org.freedesktop.DBus' Without patch: Error org.freedesktop.DBus.Error.UnixProcessIdUnknown: Could not determine PID for 'org.a11y.Bus' With patch: method return sender=org.freedesktop.DBus -> dest=:1.40 reply_serial=2 uint32 8323 ps says: 8323 ? Il 0:00.01 /usr/pkg/libexec/at-spi-bus-launcher (at-spi-bus-lau
(In reply to comment #2) > Test: .. Should have read: dbus-send --print-reply --dest=org.freedesktop.DBus /org/freedesktop/DBus \ org.freedesktop.DBus.GetConnectionUnixProcessID \ string:'org.a11y.Bus'
Comment on attachment 97249 [details] [review] NetBSD credential passing Review of attachment 97249 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +73,5 @@ > #ifdef HAVE_GETPEERUCRED > #include <ucred.h> > #endif > +#ifdef HAVE_UNPCBID > +#include <sys/un.h> Unnecessary: it is already included, unconditionally, further up the same file. @@ +1755,4 @@ > _dbus_verbose ("Failed to getsockopt() credentials, returned len %d/%d: %s\n", > cr_len, (int) sizeof (cr), _dbus_strerror (errno)); > } > +#elif defined(HAVE_UNPCBID) Surely: #elif defined(HAVE_UNPCBID) && defined(LOCAL_PEEREID) ? Or to be honest I'd be very tempted to say #elif defined(__NetBSD__) since this is different on every OS anyway. @@ +1769,5 @@ > + } > + else > + { > + _dbus_verbose ("Failed to getsockopt() credentials, returned len %d/%d: %s\n", > + cr_len, (int) sizeof (cr), _dbus_strerror (errno)); If getsockopt() returned 0 but the lengths did not match, you'll emit a misleading error message with the errno from some other syscall. Maybe this? if (getsockopt (...) != 0) { _dbus_verbose ("Failed to getsockopt(LOCAL_PEEREID): %s\n", _dbus_strerror (errno)); } else if (cr_len != sizeof (cr)) { _dbus_verbose ("Failed to getsockopt(LOCAL_PEEREID): returned %d bytes, expected %d", cr_len, (int) sizeof (cr)); } else { pid_read = ...; uid_read = ...; } If you copied that error handling from another implementation, I'd also accept a patch for that, if it has been tested on the appropriate OS (or if it's Linux, which I can test myself).
Using your patch instead of mine is fine, but if pid-passing is expected to work on NetBSD, please grab this part of my patch so the tests will fail if it doesn't: >@@ -1309,7 +1309,8 @@ check_get_connection_unix_process_id (BusContext *context, > { > #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \ > defined(__linux__) || \ >- defined(__OpenBSD__) >+ defined(__OpenBSD__) || \ >+ defined(__NetBSD__) > warn_unexpected (connection, message, "not this error"); > > goto out; ... assuming you have run the tests and they do pass :-)
Created attachment 99398 [details] [review] updated patch to implement credentials passing on NetBSD Thanks for the review - this should incorporate all of your suggestions. Re testing, my patch decreases the number of failures from 4 to 3: Without the patch, the failures are: FAIL: ../bus/test-bus FAIL: ../dbus/test-dbus FAIL: test-dbus-daemon FAIL: test-refs With the patch, the change is: PASS: test-dbus-daemon I am just doing a make check - is something else necessary when running them? Seems that credentials is icing on the cake... FAIL: ../bus/test-bus Fd 5 did not have the close-on-exec flag set! Activated service 'org.freedesktop.DBus.TestSuiteEchoService' failed: Process org.freedesktop.DBus.TestSuiteEchoService received signal 6 FAIL: ../dbus/test-dbus Not expecting error when launching nonexistent executable: org.freedesktop.DBus.Error.Spawn.ChildSignaled: Process spawn_nonexistent received signal 6 without patch FAIL: test-dbus-daemon /creds: ** Message: UnixUserID of this process is 2171 ** ERROR:../../test/dbus-daemon.c:448:test_creds: assertion failed: (seen & SEEN_PID) FAIL: test-refs /refs/connection: (./test-refs:1717): GLib-ERROR **: creating thread '': Error creating thread: Resource temporarily unavailable
(In reply to comment #6) > I am just doing a make check - is something else necessary when running them? > Seems that credentials is icing on the cake... Nothing else should be necessary, but it seems D-Bus on NetBSD has other bugs beyond this feature omission. > FAIL: ../bus/test-bus > Fd 5 did not have the close-on-exec flag set! This is a good place to start. Somewhere, BSD-specific code (or at least, code that doesn't run on modern Linux, where the tests pass) is creating a fd but not setting it close-on-exec. See also Bug #72213, Bug #77032. > Not expecting error when launching nonexistent executable: > org.freedesktop.DBus.Error.Spawn.ChildSignaled: Process spawn_nonexistent > received signal 6 The stderr from that process is probably going to /dev/null (maybe it's in the fork/exec code used to spawn subprocesses, and has already closed its stdout/stderr), but maybe enabling core dumps and inspecting the core dump with gdb would show you what went wrong? > FAIL: test-refs > /refs/connection: > (./test-refs:1717): GLib-ERROR **: creating thread '': Error creating > thread: Resource temporarily unavailable Please check that you are using a version of GLib that is not broken on NetBSD, and a version of your libc/libpthread that implements POSIX threads correctly.
Comment on attachment 99398 [details] [review] updated patch to implement credentials passing on NetBSD Review of attachment 99398 [details] [review]: ----------------------------------------------------------------- Looks good. I'll apply it next time I work on D-Bus. It will go to the 1.9.x branch because it's a new feature, but seems appropriate for a backport to 1.8.x in NetBSD. ::: dbus/dbus-sysdeps-unix.c @@ +1741,4 @@ > #endif > int cr_len = sizeof (cr); > > + if (getsockopt (client_fd, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len) != 0) I'll need to test this on Linux, but it looks as though it ought to work fine.
Created attachment 100403 [details] [review] Also look for backtrace() in libexecinfo This patch gets me backtraces in my failed tests ;-)
(In reply to comment #7) > (In reply to comment #6) > > I am just doing a make check - is something else necessary when running them? > > Seems that credentials is icing on the cake... > > Nothing else should be necessary, but it seems D-Bus on NetBSD has other > bugs beyond this feature omission. > > > FAIL: ../bus/test-bus > > Fd 5 did not have the close-on-exec flag set! > > This is a good place to start. Somewhere, BSD-specific code (or at least, > code that doesn't run on modern Linux, where the tests pass) is creating a > fd but not setting it close-on-exec. See also Bug #72213, Bug #77032. I just attached a patch to Bug #77032 which makes the close-on-exec failure go away. bus/test-bus now fails with: check_get_connection_unix_process_id:1314 received message interface "(unset)" member "(unset)" error name "org.freedesktop.DBus.Error.UnixProcessIdUnknown" on 0x7f7ff7b41590, expecting not this error 0x473f40 <_dbus_print_backtrace+0x1c> at ./../bus/test-bus 0x476abf <_dbus_abort+0x9> at ./../bus/test-bus 0x460f38 <_dbus_warn_check_failed> at ./../bus/test-bus 0x4230bb <warn_unexpected_real+0x93> at ./../bus/test-bus 0x427261 <bus_dispatch_test_conf+0x10ab> at ./../bus/test-bus 0x42869a <bus_dispatch_test+0x35> at ./../bus/test-bus 0x495270 <main+0x180> at ./../bus/test-bus (note the new backtrace given the other patch attached to this bug) Does this suggest that my LOCAL_CREDS patch isn't OK afterall?
(In reply to comment #10) > I just attached a patch to Bug #77032 which makes the close-on-exec failure > go away. bus/test-bus now fails with: > > check_get_connection_unix_process_id:1314 received message interface > "(unset)" member "(unset)" error name > "org.freedesktop.DBus.Error.UnixProcessIdUnknown" on 0x7f7ff7b41590, > expecting not this error ... > Does this suggest that my LOCAL_CREDS patch isn't OK afterall? Yes, or that it is not consistently OK (a race condition, maybe)? Attaching gdb to the test, or testing with DBUS_VERBOSE=1, might give you more information.
Comment on attachment 100403 [details] [review] Also look for backtrace() in libexecinfo This patch seems fine.
Comment on attachment 100403 [details] [review] Also look for backtrace() in libexecinfo Applied for 1.9.0. I'll hold off on applying the actual credentials-passing bit until you can confirm that it works reliably. There are other *BSD fixes in git master that might be relevant to make the other tests work, and Bug #72251 might also be relevant, so please test with (a patched version of) master rather than dbus-1.8. I don't generally apply portability fixes to stable branches, unless they're fixing a regression on a platform where upstream dbus was already actively maintained or known to work well - the more changes we make in stable branches, the more likely they are to regress on actively maintained platforms like Linux, and I want distributions to be confident that they can update within a stable branch without regressions.
Best guess so far: can it be that in the bus-test we try to find the ProcessID of the other end of a socket which hasn't done a connect(2) or a bind(2)?
(In reply to comment #14) > Best guess so far: can it be that in the bus-test we try to find the > ProcessID of the other end of a socket which hasn't done a connect(2) or a > bind(2)? I was about to say "no, it does a bind() at one end and a connect() at the other" but actually... Where possible, D-Bus normally uses Unix sockets. It can also use TCP, mostly for Windows' benefit; both are regression-tested on Unix. The server end (normally dbus-daemon in real life) does a bind() (see _dbus_listen_unix_socket() for implementation), listens for incoming connections, and eventually receives credentials. The client end (normally some application or library) does a connect() to that socket. However, the "embedded tests" (tests that are conditionally part of the library or bus daemon files rather than being a separate executable) use a separate test-only transport, found in dbus-server-debug-pipe.c. On Unix, this transport is based on socketpair() - does that meet the criteria for credentials-passing in NetBSD? Given the specific test that's failing, my guess would be "no"? If credentials-passing over a socketpair() doesn't work in NetBSD, it would be great if someone could fix that in NetBSD's kernel or libc, as appropriate: it would be good to be able to assume that all "normal socket things" work over a socketpair(). I realise that's a longer-term project than making credentials-passing work in libdbus, though! It does seem foolish that the regression tests use a transport that is never used "in real life". The reason for this bizarre choice of transport is: /* This is hard-coded in the files in valid-config-files-*. We have to use * the debug-pipe transport because the tests in this file require that * dbus_connection_open_private() does not block. */ #define TEST_DEBUG_PIPE "debug-pipe:name=test-server" which is a comment I put there after I tried and failed to remove this special case: see commit e9f0378bbf3. If socketpair() isn't expected to pass credentials, it might be better to revert the part of the patch that insists on a working GetConnectionUnixProcessID on NetBSD, and test that function in test/dbus-daemon.c instead. That test is more of a "white box" test, and operates in a much more realistic environment, with the genuine dbus-daemon, over either Unix or TCP sockets. You could base a test-case for GetConnectionUnixProcessID on the test_creds() that I already wrote GetConnectionCredentials. It should be expected to pass on FreeBSD, GNU/kFreeBSD, Linux, OpenBSD and (with your change) NetBSD. On other platforms, it should be expected to either pass and produce the correct pid (as for GetConnectionCredentials), or fail with DBUS_ERROR_UNIX_PROCESS_ID_UNKNOWN, but not succeed with any other pid or fail with any other error. It would also be good to leave a comment next to the assertion in bus/dispatch.c that credentials-passing works on "known good" platforms, maybe something like: #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \ defined(__linux__) || \ defined(__OpenBSD__) /* In principle NetBSD should also be in that list, but * its implementation of PID-passing doesn't work * over a socketpair() as used in the debug-pipe transport. * We test this functionality in a more realistic situation * in test/dbus-daemon.c. */ warn_unexpected (connection, message, "not this error"); (In reply to comment #6) > without patch FAIL: test-dbus-daemon > /creds: ** Message: UnixUserID of this process is 2171 > ** > ERROR:../../test/dbus-daemon.c:448:test_creds: assertion failed: (seen & > SEEN_PID) That is actually a bug in the test (my fault, I wrote that feature). The assertion that (seen & SEEN_PID) should be conditional on having a known-good platform, like it is in bus/dispatch.c: currently __FreeBSD__, __FreeBSD_kernel__, __linux__ or __OpenBSD__, and with your changes, __NetBSD__ can be added to the known-good list. Arguably, the check that we have received a UID should be OS-conditional as well - but I don't consider a Unix platform to be a candidate for "first-class citizen" support in D-Bus if there's no way to pass a UID around, since system bus security relies on UID-passing. So I think I'm fine with having the tests fail on (hypothetical?) Unix platforms where we can't credentials-pass the UID.
(In reply to comment #10) > I just attached a patch to Bug #77032 which makes the close-on-exec failure > go away. bus/test-bus now fails with: That wasn't correct - the patch was for systems which don't have CLOEXEC, but I do. The reason I didn't see the failure is that the tests hadn't got that far. I am trying to test the (work-in-progress) patch I'm about to attach, but can't because the tests fail first with Fd 5 did not have the close-on-exec flag set! The error seems a little odd, as the strings "fd 5" and "fd = 5" do not appear beforehand, i.e., no "file fd 5 opened". My guess is that all fds are tested, whether they were open and CLOEXEC set or not. Testing that guess isn't working either: if I comment out the warning (c.f. XXXPW in wip patch), then I run out filespace after test-bus.log reaches 5GB in size. That smells of infinite loop. Any suggestions on what to try next?
Created attachment 102072 [details] [review] wip patch with test
Created attachment 103645 [details] [review] Implement NetBSD credentials-passing now with "white box" test Switching DBUS_FATAL_WARNINGS off (because of false positive close-on-exec warnings causing an abort - same effect if I comment out the close-on-exec test loop), the attached passes test-dbus: test-dbus: running credentials tests test-dbus: checking for memleaks test-dbus: running userdb tests Current user: prlw1 homedir: /home/prlw1 gids: 0 5 Is Console user: 0 Invocation was OK: yes Is Console user 4711: 0 Invocation was OK: User "???" unknown or no memory to allocate password entry test-dbus: checking for memleaks test-dbus: running transport-unix tests test-dbus: checking for memleaks test-dbus: running keyring tests 1 keys in test test-dbus: checking for memleaks test-dbus: running sha tests SHA-1: H>SHS Type 1 Strings<H SHA-1: H>SHS Type 2 Strings<H SHA-1: H>SHS Type 3 Strings<H (ending tests due to Type 3 tests seen - this is normal) Passed the 229 SHA-1 tests in the test file test-dbus: checking for memleaks test-dbus: running auth tests Testing auth: anonymous-client-successful.auth-script anonymous-server-successful.auth-script cancel.auth-script client-out-of-mechanisms.auth-script external-failed.auth-script external-root.auth-script external-silly.auth-script external-successful.auth-script extra-bytes.auth-script fail-after-n-attempts.auth-script fallback.auth-script invalid-command-client.auth-script invalid-command.auth-script invalid-hex-encoding.auth-script mechanisms.auth-script test-dbus: checking for memleaks test-dbus: completed successfully which matches reality: uid=2171(prlw1) gid=0(wheel) groups=0(wheel),5(operator)
Now for some comments on other problems (presumably stick to this bug as they are all NetBSD related?) In the above test, I see Is Console user: 0 Invocation was OK: yes "Is Console user" should be true. uid and username are correct. The problem is that there is no DBUS_CONSOLE_AUTH_DIR = /var/run/console/ on this box. I tried configuring with --prefix=/tmp --with-console-auth-dir=/tmp/var/run/console and gmake install, mkdir /tmp/var/run/console /tmp/var/run/console was empty, and the test remained the same. I don't seem to be able to find any documentation on DBUS_CONSOLE_AUTH_DIR, e.g., what permissions it should have, should make install create it etc. Is an accurate _dbus_is_console_user vital for correct operation of dbus?
(In reply to comment #19) > Now for some comments on other problems (presumably stick to this bug as they > are all NetBSD related?) > > In the above test, I see > > Is Console user: 0 > Invocation was OK: yes > > "Is Console user" should be true. uid and username are correct. The problem > is > that there is no DBUS_CONSOLE_AUTH_DIR = /var/run/console/ on this box. > > I tried configuring with > > --prefix=/tmp --with-console-auth-dir=/tmp/var/run/console > > and gmake install, mkdir /tmp/var/run/console > > /tmp/var/run/console was empty, and the test remained the same. I don't seem > to be able to find any documentation on DBUS_CONSOLE_AUTH_DIR, e.g., what > permissions it should have, should make install create it etc. > > Is an accurate _dbus_is_console_user vital for correct operation of dbus? Am I misunderstanding and the console really is /dev/console owned by root and this is all correct?
(In reply to comment #19) > Now for some comments on other problems (presumably stick to this bug as they > are all NetBSD related?) I'd prefer to have one bug per issue, it makes it easier to track what is and isn't finished. The close-on-exec warnings should be a separate bug if they aren't already. They probably indicate a genuine bug - child processes of dbus-daemon (activated services) could start with fds open that are not meant to be there. Unfortunately, they can be quite tricky to track down, because they're "action at a distance". > Is Console user: 0 > Invocation was OK: yes > > "Is Console user" should be true. uid and username are correct. The problem > is > that there is no DBUS_CONSOLE_AUTH_DIR = /var/run/console/ on this box. "Is Console user" means "is logged-in at a local console, according to systemd-logind or ConsoleKit or or your platform's equivalent". A getty is a local console, local X11 is a local console, ssh or remote X11 is not. If there is nothing on your OS that can answer the question "is this user logged-in locally?" then the answer will always be "no". I think the point of DBUS_CONSOLE_AUTH_DIR is that ConsoleKit, or some older PAM module, or something, creates flag-files there for local logins, so we can use it to answer the question. This is all here to support D-Bus services that have security policy rules with the at_console attribute, which is somewhat deprecated anyway (Bug #39611). The modern equivalent is that services that want to allow/deny different things if a user is logged-in locally should accept all the relevant D-Bus messages, and ask policykit whether to obey or reject the request. If you have an equivalent of systemd-logind and you want to go to the effort of supporting a deprecated feature via it, please open a separate feature request.
(In reply to comment #21) > (In reply to comment #19) > > Now for some comments on other problems (presumably stick to this bug as they > > are all NetBSD related?) > > I'd prefer to have one bug per issue, it makes it easier to track what is > and isn't finished. > > The close-on-exec warnings should be a separate bug if they aren't already. > They probably indicate a genuine bug - child processes of dbus-daemon > (activated services) could start with fds open that are not meant to be > there. Unfortunately, they can be quite tricky to track down, because > they're "action at a distance". Here is why I think I am getting false positives: #include <err.h> #include <fcntl.h> #include <stdio.h> #include <unistd.h> void flagtest(char *fname) { int fd, flags; fd = open(fname, O_RDONLY | O_CLOEXEC); if (fd == -1) err(1, "open %s", fname); flags = fcntl(fd, F_GETFD); if (flags == -1) err(1, "fcntl %s", fname); printf("%s flags = 0x%x (0x%x)\n", fname, flags, flags & FD_CLOEXEC); } void emptytest() { int i, flags; /* pick a number(s), any number... */ for (i = 7; i <= 15; ++i) { flags = fcntl(i, F_GETFD); if (flags == -1) err(1, "fcntl %d", i); printf("fd %2d's flags = 0x%x (0x%x)\n", i, flags, flags & FD_CLOEXEC); } } int main() { flagtest("testfile.txt"); flagtest("testdir"); emptytest(); return 0; } $ ./cloexec testfile.txt flags = 0x1 (0x1) testdir flags = 0x1 (0x1) fd 7's flags = 0x0 (0x0) fd 8's flags = 0x0 (0x0) fd 9's flags = 0x0 (0x0) fd 10's flags = 0x0 (0x0) cloexec: fcntl 11: Bad file descriptor I didn't open fd 7,8,9,10. I think that the same is happening in dbus: checking an fd which caused a warning leading to a fatal assert doesn't appear in verbose output (as per Comment 16)
(In reply to comment #21) > The close-on-exec warnings should be a separate bug if they aren't already. Bug #83899
Comment on attachment 103645 [details] [review] Implement NetBSD credentials-passing now with "white box" test Review of attachment 103645 [details] [review]: ----------------------------------------------------------------- I'd like to land this, so I'm probably going to fix this minor issue myself. ::: test/dbus-daemon.c @@ +496,5 @@ > + g_assert_cmpstr (dbus_message_get_signature (m), ==, "u"); > + > + g_assert_true (dbus_message_get_args (m, &error, > + DBUS_TYPE_UINT32, &pid, > + DBUS_TYPE_INVALID)); This test is going to fail on [picks platform at random] Solaris. It should only assert that pid-passing works if the platform is one where we know that pid-passing works. If not, it should tolerate either working or not.
(In reply to comment #24) > This test is going to fail on [picks platform at random] Solaris. Sorry, bad example. QNX is a better one.
Created attachment 106337 [details] [review] dbus-daemon test: don't assert we pass uid/pid on unknown Unix platforms We know that Linux, FreeBSD and OpenBSD are "first class citizens" for credentials-passing, with NetBSD not far behind: people have turned up on the bug tracking system and told us that tests passed. On other Unixes, we can't really assert that it works, until someone who runs them tells us that it worked for them. Additions to these lists are welcome. --- Intended to be applied before Patrick's patch.
Created attachment 106338 [details] [review] Add NetBSD to the list of platforms where credentials-passing a pid should work --- Intended to apply after Patrick's patch.
Created attachment 106339 [details] [review] test_processid: only assert that it works if we expect it to work Otherwise, this would fail on, for instance, QNX. --- Intended to be applied after Patrick's patch.
Created attachment 106340 [details] [review] Add NetBSD to the list of platforms where credentials-passing a pid should work --- Sorry, wrong patch, here's the right one.
Review desired for my three patches here; Patrick's is Reviewed-by: me, conditional on my patches (or equivalent) also being applied.
Is reviewing open to anyone? e.g., looks good to me just from reading? (I just saw that I have the link to the attachment, rather than the link to the bug in my patch, and maybe you want to mark the wrong post-attachment as obsolete?) Sufficient for me to run tests on a fresh tree after application of each patch?
(In reply to Patrick Welche from comment #31) > Is reviewing open to anyone? e.g., looks good to me just from reading? Reviews from anyone who understands the patch are welcome, whether we consider those reviews to be sufficient for merge or not. In the case of patches written by a D-Bus maintainer (e.g. me) I've generally been going with the policy that a positive review from a non-maintainer, and no negative review from other D-Bus maintainers, is enough to merge something. > (I just saw that I have the link to the attachment, rather than the link to > the bug in my patch Linking to the bug, not the attachment, is preferred: the purpose of linking to the bug is that it lets a future maintainer re-read all the discussion that led to a patch being applied, if necessary. Linking to the attachment makes it harder to get to the discussion. (Maintainers can easily fix that when we apply your patch, though.) > Sufficient for me to run tests on a fresh tree after application of each > patch? That's sufficient to say you've tested it, but isn't review :-)
(In reply to Simon McVittie from comment #32) > Reviews from anyone who understands the patch are welcome, whether we > consider those reviews to be sufficient for merge or not. In that case, consider it reviewed. As I wrote above, from reading the patches, they look fine. > > (I just saw that I have the link to the attachment, rather than the link to > > the bug in my patch Update to follow
Created attachment 108682 [details] [review] Implement NetBSD credentials-passing - fixed link in commit msg
Comment on attachment 106337 [details] [review] dbus-daemon test: don't assert we pass uid/pid on unknown Unix platforms Review of attachment 106337 [details] [review]: ----------------------------------------------------------------- Looks good to me
Comment on attachment 106340 [details] [review] Add NetBSD to the list of platforms where credentials-passing a pid should work Review of attachment 106340 [details] [review]: ----------------------------------------------------------------- Looks good to me
Comment on attachment 108682 [details] [review] Implement NetBSD credentials-passing - fixed link in commit msg Review of attachment 108682 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-auth.c @@ +37,4 @@ > * @brief DBusAuth object > * > * DBusAuth manages the authentication negotiation when a connection > + * is first established, and also manages any encryption used over a I'll apply this now, but in future please put trivialities like this/whitespace/etc. in a separate patch that does not also make functional changes. ::: dbus/dbus-spawn.c @@ +1017,4 @@ > retval = fcntl (i, F_GETFD); > > if (retval != -1 && !(retval & FD_CLOEXEC)) > + _dbus_warn ("Fd %d did not have the close-on-exec flag set!\n", i); Same here.
(In reply to Simon McVittie from comment #28) > Created attachment 106339 [details] [review] > test_processid: only assert that it works if we expect it to work > > Otherwise, this would fail on, for instance, QNX. Review for this one too, please?
(This all looks good to apply if someone reviews Attachment #106339 [details].)
(In reply to Simon McVittie from comment #1) > Attachment #85791 [details] is as far as I got. NetBSD developers are > invited to make it work :-) > > +#ifdef __NetBSD__ > + /* Not included in AC_USE_SYSTEM_EXTENSIONS, but similar. */ > +# define _NETBSD_SOURCE > +#endif > > This part might not be necessary. If it isn't, I'd prefer not to have it. The patch uses LOCAL_PEEREID, defined in <sys/un.h> only if _NETBSD_SOURCE is defined: 55 /* 56 * Socket options for UNIX IPC domain. 57 */ 58 #if defined(_NETBSD_SOURCE) 59 #define LOCAL_CREDS 0x0001 /* pass credentials to receiver */ 60 #define LOCAL_CONNWAIT 0x0002 /* connects block until accepted */ 61 #define LOCAL_PEEREID 0x0003 /* get peer identification */ 62 #endif http://fxr.watson.org/fxr/source/sys/un.h?v=NETBSD5;im=excerpts#L55 So I guess it is necessary.
The 3 following patches look good to me: * attachment 106337 [details] [review] in comment #26 dbus-daemon test: don't assert we pass uid/pid on unknown Unix platforms * attachment 85791 [details] in comment #1 Implement NetBSD credentials-passing with LOCAL_PEEREID * attachment 106340 [details] [review] in comment #29 Add NetBSD to the list of platforms where credentials-passing a pid should work But the following ... (In reply to Simon McVittie from comment #38) > (In reply to Simon McVittie from comment #28) > > Created attachment 106339 [details] [review] [review] > > test_processid: only assert that it works if we expect it to work > > > > Otherwise, this would fail on, for instance, QNX. > > Review for this one too, please? ... does not apply on my git tree. I guess it needs to be discarded as explained in comment #29?
(In reply to Alban Crequy from comment #41) > But the following ... > > > Created attachment 106339 [details] [review] > > ... does not apply on my git tree. It should be applied after "Implement NetBSD credentials-passing with LOCAL_PEEREID". It's possiible that there's some fuzz; I have a working branch here which I'll push to my git repo shortly.
http://cgit.freedesktop.org/~smcv/dbus/log?h=netbsd&please=refresh Commits since "NEWS", in that order.
(In reply to Alban Crequy from comment #40) > The patch uses LOCAL_PEEREID, defined in <sys/un.h> only if _NETBSD_SOURCE > is defined: http://fxr.watson.org/fxr/source/sys/featuretest.h?v=NETBSD;im=3 49 […] If none of the "major" 50 * feature-test macros is defined, _NETBSD_SOURCE is assumed. We don't specifically ask for strict ANSI, POSIX or X/Open, so we get that.
(In reply to Simon McVittie from comment #43) > http://cgit.freedesktop.org/~smcv/dbus/log?h=netbsd&please=refresh > > Commits since "NEWS", in that order. That branch looks good. But I have not tested it on any platforms except Linux.
The patch whose log message is test_processid: only assert that it works if we expect it to work Otherwise, this would fail on, for instance, QNX. looks good to me too... I just grabbed the netbsd branch of git://people.freedesktop.org/~smcv/dbus, and tested on a vaguely -current NetBSD: PASS: ../bus/test-bus PASS: ../dbus/test-dbus PASS: ../bus/test-bus-launch-helper PASS: ../bus/test-bus-system PASS: test-shell PASS: test-printf PASS: test-corrupt PASS: test-dbus-daemon PASS: test-dbus-daemon-eavesdrop PASS: test-loopback PASS: test-marshal [1] Trace/BPT trap (core dumped) "${@}" >${log_fi... FAIL: test-refs PASS: test-relay PASS: test-syntax PASS: test-syslog So, the credential tests pass (and indeed the close-on-exec issues are also silenced) The refs error is: (./test-refs:7777): GLib-ERROR **: creating thread '': Error creating thread: Resource temporarily unavailable which suggests I need to do some glib debugging. I did install an ancient pygobject for the purpose of testing, as we "import gobject", and it seemed quicker to install the ancient version than work out were to put "from gi.repository import gobject". Another oddity was that if testing with DBUS_VERBOSE=1, test-dbus hangs at: 7281: [dbus/dbus-internals.c(1002):_dbus_test_oom_handling] Running once to count mallocs 7281: [dbus/dbus-sysdeps-unix.c(3230):_dbus_full_duplex_pipe] full-duplex pipe 5 <-> 6 7281: [dbus/dbus-spawn.c(579):handle_babysitter_socket] Reading data from babysitter 19886: [dbus/dbus-spawn.c(1018):do_exec] Child process has PID 19886 7281: [dbus/dbus-spawn.c(515):read_data] recorded grandchild pid 19886 7281: [dbus/dbus-spawn.c(692):_dbus_babysitter_kill_child] Got child PID 19886 for killing 29128: [dbus/dbus-spawn.c(1073):check_babysit_events] no child exited 7281: [dbus/dbus-spawn.c(595):handle_error_pipe] Reading data from child error 7281: [dbus/dbus-spawn.c(551):close_error_pipe_from_child] Closing child error 7281: [dbus/dbus-watch.c(650):dbus_watch_set_data] Setting watch fd -1 data to data = 0x0 function = 0x0 from data = 0x0 function = 0x0 Again, this doesn't seem to be related to credentials...
(In reply to Patrick Welche from comment #46) > looks good to me too... Superb, thanks. I'll merge this so it can be in 1.9.2. > The refs error is: > (./test-refs:7777): GLib-ERROR **: creating thread '': Error creating > thread: Resource temporarily unavailable > > which suggests I need to do some glib debugging. Your GLib might be broken, yeah. Its threading layer might not be 100% tested on NetBSD. Please open a separate bug if you determine that D-Bus is doing something wrong. (This is GLib, the C library that underlies both pygobject2 and pygobject3/pygi - not any particular Python thing.) > I did install an ancient > pygobject for the purpose of testing, as we "import gobject", and it seemed > quicker to install the ancient version than work out were to put "from > gi.repository import gobject". Easily fixed. I'll open a separate bug. > Another oddity was that if testing with DBUS_VERBOSE=1, test-dbus hangs at: ... > 7281: [dbus/dbus-watch.c(650):dbus_watch_set_data] Setting watch fd -1 data > to data = 0x0 function = 0x0 from data = 0x0 function = 0x0 > > Again, this doesn't seem to be related to credentials... Odd. Please open a separate bug if this recurs, but IMO it's low priority if it only happens with DBUS_VERBOSE.
(In reply to Simon McVittie from comment #47) > Easily fixed. I'll open a separate bug. https://bugs.freedesktop.org/show_bug.cgi?id=85969
Fixed in git for 1.9.2, thanks!
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.