Comment on attachment 126778 [details] [review]
patch against the master branch, #1e43857

Review of attachment 126778 [details] [review]:
-----------------------------------------------------------------

::: configure.ac
@@ +144,4 @@
>  
>  # For best security, assume that all non-Windows platforms can do
>  # credentials-passing.
> +AS_IF([test "$dbus_win" = yes || test "$dbus_darwin" = yes],

If Darwin/OS X supports AF_UNIX credentials-passing (SCM_CREDENTIALS or getpeereid() or one of the various other ways we know about), then it should force <auth>EXTERNAL</auth> just like Linux does. That mechanism is by far the best one available to us.

If Darwin/OS X supports AF_UNIX credentials-passing but not by one of the code paths we know about, please contribute code to add a Darwin code path to dbus-sysdeps-unix instead of disabling this. That should be a separate commit.

If Darwin/OS X does not support AF_UNIX credentials passing at all, then not using <auth>EXTERNAL</auth> should be a separate commit, and the comment just above here also needs changing.

@@ +201,5 @@
>  AC_ARG_WITH(system-socket, AS_HELP_STRING([--with-system-socket=[filename]],[UNIX domain socket for systemwide daemon]))
>  AC_ARG_WITH(console-auth-dir, AS_HELP_STRING([--with-console-auth-dir=[dirname]],[directory to check for console ownerhip]))
>  AC_ARG_WITH(console-owner-file, AS_HELP_STRING([--with-console-owner-file=[filename]],[file whose owner determines current console owner]))
> +AC_ARG_WITH(launchd-agent-dir, AS_HELP_STRING([--with-launchd-agent-dir=[dirname]],[directory to put the launchd agent (session bus) (default: /Library/LaunchAgents)]))
> +AC_ARG_WITH(launchd-daemon-dir, AS_HELP_STRING([--with-launchd-daemon-dir=[dirname]],[directory to put the launchd daemon (system bus) (default: /Library/LaunchDaemons)]))

These are "underquoted". Please use one level of [] per level of () (wrap both the arguments of AC_ARG_WITH in one more [] than you have). AC_ARG_WITH([test_user]), below, is in the correct form.

(I know this is not consistently correct across all of configure.ac; I'm applying the rule that new code is held to higher standards than current code.)

You can use

AC_ARG_WITH([launchd_agent_dir], [help], [], [with_launchd_agent_dir=/Library/LaunchAgents])

to set a default if --with-launchd-agent-dir is not given (look at the AC_ARG_WITH([valgrind]) to see an example of setting a default like that).

I don't see any uses of $with_launchd_agent_dir at all? Please don't add options that don't actually do anything.

I don't see any uses of $LAUNCHD_DAEMON_DIR either, other than the message at the end.

@@ +1145,5 @@
>  
>  AC_SUBST(LAUNCHD_AGENT_DIR)
>  
> +#### Directory to place launchd daemon file
> +if test "x$with_launchd_daemon_dir" = "x"; then

Should be

AS_IF([test ...], [L_D_D=...], [L_D_D=...])

but if you set a default in the AC_ARG_WITH call and use AC_SUBST([LAUNCHD_DAEMON_DIR], [$with_launchd_daemon_dir]) then you won't need this block at all.

@@ +1867,4 @@
>  bus/messagebus
>  bus/messagebus-config
>  bus/org.freedesktop.dbus-session.plist
> +bus/org.freedesktop.dbus-system.plist

You haven't provided this file. The build will fail.

::: dbus/dbus-bus.c
@@ +520,5 @@
>    _DBUS_UNLOCK (bus);
> +#ifdef DBUS_ENABLE_LAUNCHD
> +  _dbus_verbose ("internal_bus_get(type=%d,private=%d) : found and registered connection to %s",
> +         type, private, address);
> +#endif

Is this extra verbosity really particularly useful?

If it is, enable it unconditionally. _dbus_verbose() compiles to nothing unless you ./configure --enable-verbose anyway.

If it isn't, remove it.

::: dbus/dbus-server-unix.c
@@ +266,3 @@
>            _dbus_set_bad_address (error, "launchd", "env", NULL);
>            return DBUS_SERVER_LISTEN_DID_NOT_CONNECT;
> +#endif

"#if 0" is never a good sign. If code is unnecessary or unwanted, remove it.

@@ +281,5 @@
> +            {
> +              _DBUS_ASSERT_ERROR_IS_SET(error);
> +              return DBUS_SERVER_LISTEN_DID_NOT_CONNECT;
> +            }
> +#endif

"#if 0" is never a good sign. If code is unnecessary or unwanted, remove it.

If you are deliberately ignoring an error, you need to free it; otherwise the memory is leaked.

@@ +286,2 @@
>          }
> +      // try a hardcoded traditional address as a fallback

You seem to be changing this function so that

    dbus-daemon --address=launchd:

is equivalent to

    dbus-daemon --address=unix:tmpdir=$DBUS_SESSION_SOCKET_DIR

but that makes no sense. If --address=launchd: is valid, you don't need to transform it. If it is invalid, it is correct for dbus-daemon --address=launchd: to exit unsuccessfully.

It is fine to have address strings that are defined to be incorrect/invalid. dbus-daemon --address=unix: isn't allowed either.

I don't think any of your changes to this function are actually necessary?

::: dbus/dbus-sysdeps-unix.c
@@ +3928,4 @@
>  
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
> +  const char *fromEnv = getenv (launchd_env_var);

I don't understand why you look for the same information in either this process's environment or the launchd environment? Surely one of them is correct, and the other isn't?

Use _dbus_getenv() so that you aren't deliberately bypassing the safety check that prevents trusting our environment when running setuid.

Our coding style is to use variable_names_like_this, not variableNamesLikeThis.

@@ +4136,5 @@
> +      return TRUE;
> +    }
> +// dbus can function without launchd like it does on other Unix versions, even when
> +// it will use launchd by default (on Mac). So there is no need to disable the
> +// non-launchd code path below.

If you are deliberately ignoring a non-null error, you need to free it; otherwise it's a memory leak.

Please use /* */ C-style comments, not // C++-style.

@@ +4137,5 @@
> +    }
> +// dbus can function without launchd like it does on other Unix versions, even when
> +// it will use launchd by default (on Mac). So there is no need to disable the
> +// non-launchd code path below.
> +#endif

Can't you just delete all this launchd-specific code, and fall through to the global default, which is normally "autolaunch:", but in configure.ac you changed it so it would be "launchd:env=DBUS_LAUNCHD_SESSION_BUS_SOCKET"?

(In reply to René J.V. Bertin from comment #0)
> patch against the master branch, #1e43857

Please use "git format-patch" to attach a complete patch with a description (<http://chris.beams.io/posts/git-commit/>).

> 1- being able to launch a session daemon when logged in remotely to OS
> X/macOS, i.e. when launchd cannot be used. In that case the OS is used as a
> traditional Unix, typically with X11 providing remote display, and there are
> no technical reasons why DBus wouldn't support this mode of operation
> ("shouldn't" may be a different thing).

If this is something you want, then you might want to make the default session bus connect address be

launchd:env=whatever,autolaunch:

so that the D-Bus client library will:

* first look for $DBUS_SESSION_BUS_ADDRESS, and if that is set, use it unconditionally (and if it fails, connecting will just fail - this is intended); if it is unset, continue
* then try launchd:env=whatever; if that succeeds, use it; if it fails, continue
* then try X11 autolaunch; if *that* succeeds, use it; if it fails, continue
* fail to connect

> Given the context the exact value of that entry is without importance. On
> Linux it would be set to "autolaunch:"; in the attached patch I set it to
> "launchd:env=DBUS_LAUNCHD_SESSION_BUS_SOCKET" because that appears to be an
> existing convention.

As far as I can see, if you set DBUS_SESSION_BUS_CONNECT_ADDRESS correctly, and delete the weird launchd-specific code so that it *is* important (in particular so it actually gets used), then everything will work better with less code. I like fixing bugs by deleting code.

> - should the org.freedesktop.dbus-system.plist launchd profile provided by
> my patch be installed by default or should it be optional or an operation to
> be done manually? Note that installing it doesn't load profile.

Sorry, I don't use or develop on a Mac. You will need to ask other Mac people what is most appropriate here, and bring us a consensus. For example, consider talking to the maintainers of dbus in Homebrew and/or Fink.

> - should _dbus_lookup_launchd_socket() indeed check the value of
> DBUS_LAUNCHD_SESSION_BUS_SOCKET in the environment, and if so should that
> value have priority over the value obtained from launchctl? The intention I
> had with the addition was to give a similar mechanism to override the
> "default" session bus as can be achieved on Linux by overriding
> DBUS_SESSION_BUS_ADDRESS.

I'm not sure why you wouldn't do this by overriding DBUS_SESSION_BUS_ADDRESS, exactly the same as you'd do on Linux?

-- 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/157.