[Nasty sketch] Stop crashing High Sierra #1617

copumpkin · 2017-10-18T00:16:59Z

This is a nasty approach and I don't like it, but given that the other kill(-1, SIGKILL) approach crashes macOS 10.13 and we don't know when Apple will fix their kernel, we're left with shitty options.

I threw this together rather hastily to get people thinking about it. It's untested (I don't have a High Sierra or even a multi-user install handy right now) and I'm on a train and can't do much, but I figured I'd throw up the sketch and see what y'all thought of the approach.

cc @shlevy @edolstra @LnL7 @grahamc @matthewbauer

copumpkin · 2017-10-18T00:18:12Z

This compiles so ship it, right? 😄

copumpkin · 2017-10-18T00:20:21Z

src/libutil/util.cc

+        vector<struct kinfo_proc> proc_list(length);
+
+        newlength = length;
+        err = sysctl((int *)name, 4, (void *)proc_list.data(), &newlength, NULL, 0);


I was originally going to check if length == newlength but then couldn't see a reason to...

Can you use a const_cast here with a comment reassuring us that the name argument won't be modified based on some manpage or whatever?

This is a nasty approach and I don't like it, but given that the other `kill(-1, SIGKILL)` approach crashes macOS 10.13 and we don't know when Apple will fix their kernel, we're left with shitty options.

shlevy · 2017-10-18T00:53:39Z

src/libutil/util.cc

+    int err = 0;
+    size_t length = 0, newlength = 0;
+
+    // XXX: Should this be KERN_PROC_RUID? EUID vs. real UID


They should be identical for normal programs run by the user, so whichever one is set to the nixbld user for a setuid program, right?

grahamc · 2017-10-18T00:55:26Z

src/libutil/util.cc

+        // @grahamc points out https://utcc.utoronto.ca/~cks/space/blog/unix/ProcessKillingTrick
+        for (auto proc : proc_list) {
+            kill(proc.kp_proc.p_pid, SIGSTOP);
+        }


It is possible that between listing the processes and the SIGSTOP, the process dies and the PID is reused by another user. After the SIGSTOP, we can take a look to see if the PID is owned by the build user we're wanting to kill. If it is our builder, SIGKILL it. If not, SIGCONT it.

@dezgeg was kind enough to point out SIGCONT to a SIGSTOP'd process, which was SIGSTOP'd before we got to it is rude... which is true, but jeeze, that means the processes exited, the pid recycled, started, and SIGSTOP'd within such a brief period of time. Still rude, but we tried (?)

Maybe we should do the kill's as the nixbld user instead?

Then if it fails due to EPERM, we just continue on our way

@copumpkin Probably this whole loop should just be done as the nixbld user (excluding the current process, of course)

It'd be really helpful if we could SIGKILL -1 and only kill things we own.

Sounds good, I'll run as subprocess again

But actually, if I run as subprocess, we need to start worrying about our siblings killing us again...

But if our siblings kill us we know there's a rogue

shlevy · 2017-10-18T00:56:22Z

src/libutil/util.cc

+        tries--;
+
+        // First call with NULL to get number of processes. Yeah yeah TOCTOU is fun, but we check
+        // that it hasn't changed later and keep going until it's 0


We don't check that it hasn't changed later, right?

Yeah I don't. Should I? My first thought was that I'd need to, but then I thought some more and convinced myself I wouldn't.

Nah, shouldn't be necessary, just need to update the comment

shlevy · 2017-10-18T00:58:15Z

src/libutil/util.cc

+        if (err)
+            throw SysError("calling sysctl to get process list");
+
+        // @grahamc points out https://utcc.utoronto.ca/~cks/space/blog/unix/ProcessKillingTrick


A brief description of why this helps inline would be nice.

shlevy

Other than my minor nits, 🚢

copumpkin · 2017-10-18T01:09:56Z

Thanks for all the feedback! Will fix when train wifi wakes up again or tomorrow

matthewbauer · 2017-10-18T01:23:36Z

I think you’re also going to need to handle this SIGKILL:

nix/src/libstore/build.cc

Line 1013 in 1dd29d7

::kill(-pid, SIGKILL); /* ignore the result */

copumpkin · 2017-10-18T02:22:22Z

@matthewbauer I was never able to get it to die when killing a process group. Were you? I left it running for a while but only ever saw crashes from kill(-1, ...)

matthewbauer · 2017-10-18T02:26:37Z

@copumpkin Ok maybe not... I guess I was thinking more had to be done based on 3cc1b57 still leading to crashes for some people. That was before we saw the issue with plain old kill(-1, SIGKILL) though.

matthewbauer · 2017-11-03T19:55:42Z

@copumpkin I've got this branch installed and running on High Sierra. So far so good 💯. Will report back if I get a system lock again.

copumpkin · 2017-11-03T20:12:15Z

Nice!

matthewbauer · 2017-11-03T21:11:34Z

src/libutil/util.cc

+
+        // First call with NULL to get number of processes. Yeah yeah TOCTOU is fun, but we check
+        // that it hasn't changed later and keep going until it's 0
+        err = sysctl((int *)name, 4, NULL, &length, NULL, 0);


Does this work for you @copumpkin ?

Trying this little program is giving really big length values:

#include <sys/sysctl.h> #include <stdio.h> #include <string.h> #define UID 30001 int main() { size_t length = 0; int errno = 0; static const int name[4] = { CTL_KERN, KERN_PROC, KERN_PROC_UID, UID }; errno = sysctl((int *)name, 4, NULL, &length, NULL, 0); printf("uid:%i length:%zu err:%s\n", UID, length, strerror(errno)); }

Maybe I screwed it up! I haven't actually run this yet 😄

Haha that's fine. I'll go through it and see whats works. Before now I had hoped apple would just fix the issue but 10.13.1 still has the issue. Google has the same line here though so maybe my c program is what's wrong:

https://chromium.googlesource.com/chromium/src/base/+/master/process/process_iterator_mac.cc#27

Well each process entry is actually a struct so that might be fairly large. How big are the values you're seeing? Also, now that I think about it, my vector might be too big, since I should probably divide it by the size of the struct.

Yep that looks more like it (ceacd3e):

Although it looks like it's the same regardless of the user:

euid:501 uid:501 length:344 err:Undefined error: 0 euid:501 uid:30001 length:344 err:Undefined error: 0

Weird, maybe we'll just need to filter by hand based on the struct?

Or maybe try KERN_PROC_RUID

matthewbauer · 2017-11-05T02:22:10Z

Hey @copumpkin,
I've updated a few things here: https://github.com/matthewbauer/nix/tree/shitty-kill-processes

So far so good. Going to use it for a little while until I say it's worth merging but haven't had the hanging yet. (lot's of weird High Sierra stuff though- but like I've said I'd blame it on High Sierra's bugginess first).

Testing directions

$ git clone https://github.com/matthewbauer/nix
$ git checkout shitty-kill-processes
$ nix-build release.nix -A build.x86_64-darwin
$ sudo su
# nix-env -i ./result
# reboot

copumpkin commented Oct 18, 2017

View reviewed changes

[Nasty sketch] Stop crashing High Sierra

e69a111

This is a nasty approach and I don't like it, but given that the other `kill(-1, SIGKILL)` approach crashes macOS 10.13 and we don't know when Apple will fix their kernel, we're left with shitty options.

copumpkin force-pushed the shitty-kill-processes branch from bd4370a to e69a111 Compare October 18, 2017 00:35

shlevy reviewed Oct 18, 2017

View reviewed changes

grahamc reviewed Oct 18, 2017

View reviewed changes

shlevy reviewed Oct 18, 2017

View reviewed changes

shlevy approved these changes Oct 18, 2017

View reviewed changes

matthewbauer reviewed Nov 3, 2017

View reviewed changes

copumpkin closed this Dec 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Nasty sketch] Stop crashing High Sierra #1617

[Nasty sketch] Stop crashing High Sierra #1617

copumpkin commented Oct 18, 2017

copumpkin commented Oct 18, 2017

copumpkin Oct 18, 2017

shlevy Oct 18, 2017

copumpkin Oct 18, 2017

shlevy Oct 18, 2017

copumpkin Oct 18, 2017

grahamc Oct 18, 2017

shlevy Oct 18, 2017

shlevy Oct 18, 2017

grahamc Oct 18, 2017

shlevy Oct 18, 2017

grahamc Oct 18, 2017

copumpkin Oct 18, 2017

copumpkin Oct 18, 2017

shlevy Oct 18, 2017

shlevy Oct 18, 2017

copumpkin Oct 18, 2017

shlevy Oct 18, 2017

shlevy Oct 18, 2017

shlevy left a comment

copumpkin commented Oct 18, 2017

matthewbauer commented Oct 18, 2017

copumpkin commented Oct 18, 2017

matthewbauer commented Oct 18, 2017 •

edited

Loading

matthewbauer commented Nov 3, 2017 •

edited

Loading

copumpkin commented Nov 3, 2017

matthewbauer Nov 3, 2017

copumpkin Nov 3, 2017

matthewbauer Nov 3, 2017 •

edited

Loading

copumpkin Nov 3, 2017

matthewbauer Nov 3, 2017

copumpkin Nov 3, 2017

copumpkin Nov 3, 2017

matthewbauer commented Nov 5, 2017 •

edited

Loading

[Nasty sketch] Stop crashing High Sierra #1617

[Nasty sketch] Stop crashing High Sierra #1617

Conversation

copumpkin commented Oct 18, 2017

copumpkin commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlevy left a comment

Choose a reason for hiding this comment

copumpkin commented Oct 18, 2017

matthewbauer commented Oct 18, 2017

copumpkin commented Oct 18, 2017

matthewbauer commented Oct 18, 2017 • edited Loading

matthewbauer commented Nov 3, 2017 • edited Loading

copumpkin commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewbauer Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewbauer commented Nov 5, 2017 • edited Loading

matthewbauer commented Oct 18, 2017 •

edited

Loading

matthewbauer commented Nov 3, 2017 •

edited

Loading

matthewbauer Nov 3, 2017 •

edited

Loading

matthewbauer commented Nov 5, 2017 •

edited

Loading