Bug 69702

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!