Bug#851148: tracker-extract: dumps core repeatedly if seccomp raises SIGSYS

[Ben Longbons]

FYI, your mailer is not using TLS, so it's getting marked a spam.


On Thu, Jan 12, 2017 at 5:07 AM, Simon McVittie  wrote:
>> should log those details
>
> Logging in response to an async signal is problematic: it is not safe
> to use anything much more complicated than a syscall in a signal
> handler (in particular, stdio or GLib logging is a bad idea and
> could deadlock). I think the correct thing here is for tracker-extract
> to just crash, and let system-level diagnostic tools like
> systemd-coredump work out why.

It's quite true that doing anything *complicated* is forbidden. But
there's no need to be complicated.

Just write(2) a newline, a handful of fixed strings, and the hex value
of the syscall and instruction. snprintf(3) isn't safe, but converting
an integer to hex by hand is trivial anyway, compared to using a
seccomp sandbox. Personally, I would've just chrooted (having userns
these days makes sandboxing *so* much easier since you don't need
permissions to become a new "root").

>> During a recent apt run, my system became almost completely
>> unresponsive.
>
> I suspect that the thing stalling your system here is actually
> the repeated core-dump processing, not the repeated
> tracker-extract startup - at least, that has been my experience when
> dealing with repeatedly crashing software. systemd-coredump uses
> relatively expensive compression.

True, but the crashing application is still responsible for the
consequences, especially when it's something that nobody ever asked
for anyway.

Bug#851148: tracker-extract: dumps core repeatedly if seccomp raises SIGSYS

[Simon McVittie]

On Thu, 12 Jan 2017 at 13:07:49 +, Simon McVittie wrote:
> * On systems with dbus-user-session installed, `systemd --user` restarts
>   tracker-extract on failure, as requested by the systemd unit.
>   This is easy to avoid by adding SIGSYS to RestartPreventExitStatus,
>   and I'm trying a patch that does that.

See attached. On my system, this delays the restart by a couple of
seconds, which hopefully mitigates the load issue.

S
>From 8655eb4d4891a8276d5443d36ce0d2e1de70bf4b Mon Sep 17 00:00:00 2001
From: Simon McVittie 
Date: Thu, 12 Jan 2017 12:50:46 +
Subject: [PATCH 3/4] Don't immediately restart tracker-extract on SIGSYS

Mitigates: #851148
---
 debian/changelog |  1 +
 ...mediately-restart-tracker-extract-on-SIGSYS.patch | 20 
 debian/patches/series|  1 +
 3 files changed, 22 insertions(+)
 create mode 100644 debian/patches/Don-t-immediately-restart-tracker-extract-on-SIGSYS.patch

diff --git a/debian/changelog b/debian/changelog
index e999c4aa2..5dc1f9047 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -6,6 +6,7 @@ tracker (1.10.3-1.1) UNRELEASED; urgency=medium
 introduced in 1.10.2.
   * Add patches from upstream to make the sandbox allow more harmless
 syscalls (Closes: #848842, #849936, LP: #1649035, LP: #1649004)
+  * Don't immediately restart tracker-extract on SIGSYS. Mitigates: #851148
 
  -- Simon McVittie   Thu, 12 Jan 2017 12:11:44 +
 
diff --git a/debian/patches/Don-t-immediately-restart-tracker-extract-on-SIGSYS.patch b/debian/patches/Don-t-immediately-restart-tracker-extract-on-SIGSYS.patch
new file mode 100644
index 0..528b8ead7
--- /dev/null
+++ b/debian/patches/Don-t-immediately-restart-tracker-extract-on-SIGSYS.patch
@@ -0,0 +1,20 @@
+From: Simon McVittie 
+Date: Thu, 12 Jan 2017 12:42:38 +
+Subject: Don't immediately restart tracker-extract on SIGSYS
+
+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=851148
+---
+ src/tracker-extract/tracker-extract.service.in | 3 ++-
+ 1 file changed, 2 insertions(+), 1 deletion(-)
+
+diff --git a/src/tracker-extract/tracker-extract.service.in b/src/tracker-extract/tracker-extract.service.in
+index b7bc6c140..415e130e1 100644
+--- a/src/tracker-extract/tracker-extract.service.in
 b/src/tracker-extract/tracker-extract.service.in
+@@ -7,4 +7,5 @@ BusName=org.freedesktop.Tracker1.Miner.Extract
+ ExecStart=@libexecdir@/tracker-extract
+ Restart=on-failure
+ # Don't restart after tracker daemon -k (aka tracker-control -k)
+-RestartPreventExitStatus=SIGKILL
++# Don't restart after seccomp raises SIGSYS either
++RestartPreventExitStatus=SIGKILL SIGSYS
diff --git a/debian/patches/series b/debian/patches/series
index 99e36a328..d3e275326 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -3,3 +3,4 @@ libtracker-common-Whitelist-dup-dup2-dup3.patch
 libtracker-common-Whitelist-umask.patch
 libtracker-common-Handle-mlock-munlock-syscalls.patch
 libtracker-common-Allow-querying-process-stats-limits.patch
+Don-t-immediately-restart-tracker-extract-on-SIGSYS.patch
-- 
2.11.0

Bug#851148: tracker-extract: dumps core repeatedly if seccomp raises SIGSYS

[Simon McVittie]

On Thu, 12 Jan 2017 at 12:44:08 +, Simon McVittie wrote:
> On Mon, 19 Dec 2016 at 22:39:29 -0800, Ben Longbons wrote:
> > Remember, SIGSYS is catchable, and the siginfo_t contains details. If a
> > program is going to use `seccomp`, it should log those details, before
> > exiting with a value that tells systemd not to restart it.
> 
> I've cloned a new bug for this.

(It is #851148.)

This restart behaviour is less straightforward than you might think.
There are two things that restart tracker-extract:

* On systems with dbus-user-session installed, `systemd --user` restarts
  tracker-extract on failure, as requested by the systemd unit.
  This is easy to avoid by adding SIGSYS to RestartPreventExitStatus,
  and I'm trying a patch that does that.

* On all systems, tracker-miner-fs watches the tracker-extract process
  and sends a D-Bus message to restart it, either started by
  `systemd --user` (with dbus-user-session) or by dbus-daemon (without).
  This is deliberate: tracker-extract only tries indexing any given
  file a limited number of times, and will eventually skip over it.

Processing each file n times doesn't do much to mitigate missing benign
syscalls in the seccomp sandbox, because n tries * m files is still a
large number: the assumption is that the typical failure mode is a
particular file that trips a bug in the extractor libraries.

The introduction of the seccomp sandbox is an unusual situation that
caused extraction of broad equivalence classes of files to fail,
breaking that assumption. There are several patches upstream that
hopefully resolve this, which I'm testing now.

I agree with the maintainer that this is not release-critical,
although I personally think the actual crashes might be.

> should log those details

Logging in response to an async signal is problematic: it is not safe
to use anything much more complicated than a syscall in a signal
handler (in particular, stdio or GLib logging is a bad idea and
could deadlock). I think the correct thing here is for tracker-extract
to just crash, and let system-level diagnostic tools like
systemd-coredump work out why.

> During a recent apt run, my system became almost completely
> unresponsive.

I suspect that the thing stalling your system here is actually
the repeated core-dump processing, not the repeated
tracker-extract startup - at least, that has been my experience when
dealing with repeatedly crashing software. systemd-coredump uses
relatively expensive compression.

S