Bug 32851

Created attachment 41676 [details] [review]
Patch from Kimmo Hämäläinen via dbus/1.4.1-0maemo0

Note that I haven't reviewed this patch yet and have no opinion on whether it's the best approach.

Looks good, but the oom_adj interface is already obsolete, hence this should probably be updated to oom_score_adj before it is merged.

Also, O_CLOEXEC should be used and O_SYNC be dropped, and the whole thing should be in an #ifdef __linux__ block.

Created attachment 124402 [details] [review]
activation: set children oom_score_adj to 0

From: WaLyong Cho <walyong.cho@samsung.com>

If dbus is running as systemd service, dbus daemon is running with
oom_score_adj -900 by OOMScoreAdjust=-900. And children will also have
same value with dbus daemon.
To avoid this, set the child itself values after fork () to 0.

---

From the mailing list.

Comment on attachment 124402 [details] [review]
activation: set children oom_score_adj to 0

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

This is clearly better than what was in Maemo, and also better than what we have now, assuming it compiles successfully on every Linux platform we care about.

::: dbus/dbus-spawn.c
@@ +1359,5 @@
>  	}
>        else if (grandchild_pid == 0)
>        {
> +#ifdef __linux__
> +          int fd = open ("/proc/self/oom_score_adj", O_WRONLY | O_CLOEXEC);

To be fully portable, this should ideally be something like

    int fd = -1;

#ifdef O_CLOEXEC
    fd = open (..., O_WRONLY | O_CLOEXEC);
#endif

    if (fd < 0)
      {
        fd = open (..., O_WRONLY);
        _dbus_fd_set_close_on_exec (fd);
      }

    if (fd >= 0)
      ... continue, do the write ...

However, perhaps we don't actually care about Linux platforms that don't have O_CLOEXEC any more; it was introduced in Linux 2.6.23. Our policy is that we don't support platforms whose vendor no longer provides security support, so if nobody ships Linux kernels or glibc that lack O_CLOEXEC in a security-supported distribution, we don't need to bother with this.

#if (defined(__linux__) && defined(O_CLOEXEC)) would be a simpler way to make sure this can never break compilation.

@@ +1363,5 @@
> +          int fd = open ("/proc/self/oom_score_adj", O_WRONLY | O_CLOEXEC);
> +
> +          if (fd >= 0)
> +            {
> +              write (fd, "0", sizeof (char));

This is going to provoke compiler warnings, because write() is declared with __attribute__ ((__warn_unused_result__)). Please use

if (write (fd, "0", sizeof (char)) < 0)
  {
    /* ignore error, this is non-critical */
  }

to show that we've thought about the correct error-handling, or replace the comment with some sort of error-handling if necessary. (But I suspect that the correct error-handling here is indeed to ignore it and just carry on.)

Created attachment 124676 [details] [review]
v2: activation: set children oom_score_adj to 0

Comment on attachment 124676 [details] [review]
v2: activation: set children oom_score_adj to 0

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

This looks good, thanks. I'll merge it soon.

I can't help wondering whether this should be in child_setup() in bus/activation.c: it's the dbus-daemon that has elevated OOM-killer protection, so it's the dbus-daemon, not the library code, that should take responsibility for resetting that attribute. But it looks as though we only use dbus-spawn in the dbus-daemon and tests anyway, so no big deal.

Thanks, fixed in git for 1.10.10, 1.11.4