Kill init by touching a bunch of files
rachelbythebay.com
rachelbythebay.com
Obviously, the patch sucks -- it just ignores the error, instead of doing the right thing.
OTOH, init not shitting the bed and taking your whole OS down is probably a good thing, even if getting there is imperfect.
Technically the patch isn't rejected (and I'm kinda peeved TFA claims it is). It's in limbo, waiting for further action from the submitter or an other contributor: it's marked as needing improvements since it hides the issue under the rug instead of reporting it to the caller/user.
This is a rejected patch: https://code.launchpad.net/~jamesodhunt/libnih/bug-776532/+m..., it has a "Status: Rejected" set (and a disapproving review).
Although of course the patch could have been merged and improved later, so that libnih wouldn't blow up the whole system in case of inotify overflow.
Looks to me that Upstart kept the lesser of two evils. I'd reject that patch too.
It does not restart, it locks up with a mostly useless error message, then, maybe, at one point, possibly restarts. The restart is not a policy decision it's a side effect of the box being dead. Here's the better option: bubble up the issue to the caller and let it decide what to do.
> Looks to me that Upstart kept the lesser of two evils.
Upstart didn't do anything.
> I'd reject that patch too.
The patch isn't rejected.
Just because the reject status wasn't used does not mean it wasn't rejected. The patch, as it was written, was rejected. It won't be used and the recommendation was a complete rewrite in a completely different direction.
As it stands, the communication was:
1. There's a problem, here's a patch.
2. NAK, because (valid reasons).
3. Radio silence.
Perhaps better behaviour would have been:
2. NAK, because (valid reasons). However, I acknowledge the problem; perhaps you can fix it (some other way).
This way, the discussion is more likely to keep going and end up with a proper fix to the issue.
1. There's a problem, here's a patch.
2. We won't apply it because it doesn't fix it perfectly. Sure it's better than what we did, and you offered the patch for free out of the kindness of your heart, but we aren't going to apply it until you work on it some more.
3. But... it's better in every way!
If you write a patch for a project that is an improvement in every way, but not yet perfect and they don't apply it... Well you're probably not going to spend much more time helping that project are you?
And this is where we disagree. I believe it's worse. The original crashes the system, which is really bad. The cases where this happens are few, and people are going to be aware of what they did to cause it (unzipping a bunch of files in there was suggested as a trigger). With the proposed patch, nothing will happen. The user will not be aware of the issue and the system will not update with the changes. The user may not even notice their action had no effect and if they do, it'll be a harder mystery than the crash to figure out.
Some prefer the system to crash than silently ignore input. Error codes and messages are better than either of those.
If the kernel drops and never returns a notification, then init has to know that it missed some in order to operate under the correct set of init files. That requires a combination patch to init and to libnih.
If the kernel gets around to notifying anyway, just more slowly, then you can safely ignore it, init will eventually get the message the 'regular' way and you're done.
Given the bug, the next step might be to see if systemd suffers a similar challenge in the presence of a lot of config changes.
Many developers ignore this, so it's not really surprising that this has happened with inotify too. It's mentioned that a patch wasn't accepted, but it was with good reason - it doesn't fix the problem (by traversing the directory).
I'd much prefer to accept the patch with a followup request, tracking issue, or TODO. There wass good reason to not consider the issue resolved, but I'm not of the opinion there was good reason to reject the patch either.
I'm not entirely sure I'd agree with that line of reasoning, to be honest. While the crash is a big problem and falls into the category of "obscure enough" problems that might leave your sysadmins scratching their heads for quite some time (especially because of the contrived and terrible crash report message), I would much prefer my system to crash and halt instead of failing to recognize/load a config file. Not loading config files properly (and failing silently, at that) means that you might be exposed to security attacks, your system might be in an inconsistent state until the next reboot and you might not even know it. I'll take a failure over prolonged inconsistent state of a machine any day. That's what redundancy solves after all.
The patch should log a warning/error and avoid the crash. It does not preempt an optimal solution, by traversing the file tree, being developed afterwards.
In this case, by my standards, it already is. Some relatively unrelated bit of code is later failing loudly.
> This is a case where the best is enemy of the good.
Neither best nor good happened in this case - so I'd flip your terms around. The best of intentions and standards prevented good.
> The patch should log a warning/error and avoid the crash. It does not preempt an optimal solution, by traversing the file tree, being developed afterwards.
On this we can agree wholeheartedly. That'd be a great followup changelist in lieu of doing the optimal solution, assuming the latter is too time consuming.
This is a false dichotomy anyway: The solution is not to not apply the patch if the submitter doesn't provide an alternative, but recognise that this is a potentially serious problem and fix it anyway.
There are multiple alternatives:
* Fix libnih.
* Fork libnih for upstart, and fix it there.
* Ditch libnih for this, and catch the problem yourself.
* fork() and treat the child failing as a sign you need to rescan the entire config directory after restarting it.
* Regularly rescanning the directory anyway, on the assumption that Things-Go-Wrong and events will get missed out for some reason or the other.
More than one of those can be applied at once. Personally I'd opt for the two last in combination.
With the kernel panic, you'll be ending up in an inconsistent state even after the reboot - assuming it reboots - when it happens in the middle of un-taring your configuration files.
Plus, there are modern features in kernels that allow you to audit your kernel panics, figure out what is wrong and, hopefully, everything should be automatically reverted back to how it was thanks to backups and journaling on the file system (to prevent corrupted images, that is).
Of course this won't automatically fix your problems for you or finish your un-taring of config files, but that's why we have sysadmins, isn't it?
Rather than fix an issue that can crash a whole box, hold out until the "right thing" is done.
I don't think either are great here, so the only real way to fix this is to, well, fix it!
The patch can not fix the problem: upstart can't scan the directory (or ignore the overflow, or handle it however it would) because libnih blows up without yielding to upstart. It does not call a NihWatch handler and does not return an error code.
So the first step is to have libnih at least not blow up (which the patch did), ideally notify the user (which the patch didn't). The purpose of the patch was not to fix the issue, it was to fix libnih's broken handling of inotify.
It isn't up to libnih to handle the overflow, but the problem right now is in libnih itself[0] and only libnih can fix that.
[0] and is twofold: libnih blows up on inotify overflow instead of not blowing up on a relatively normal (if rare) situation, and consequently (and obviously) libnih doesn't provide any way for its caller to handle the overflow.
That would give upstart a way of handling the overflow and other (unknown) potential problems: rescan the config if the config watcher dies (after respawning the watcher).
Although then you have the problem of the notification channel between the daemon and Upstart overflowing, i suppose.
> libnih is a small library for C application development containing functions that, despite its name, are not implemented elsewhere in the standard library set.
Here's the core code, for the curious: http://bazaar.launchpad.net/~scott/libnih/trunk/files/head:/....
I found it interesting that it uses a space after the unary ! operator in C, which (to me), provided yet another way of writing various (very common) tests. For instance "if(! fp)" after trying to open a file.
I'd be curious to know if systemd is tickled by this one weird trick to crash your server. It does work with inotify (and other fun newish kernel techs), so it would be subject to the inotify queue length...but what'll happen when it hits it?
$ git clone git://anongit.freedesktop.org/systemd/systemd systemd-git
$ cd systemd-git
$ grep -r "assert(" | wc -l
7771
/* We override the glibc assert() here. */
#undef assert
#ifdef NDEBUG
#define assert(expr) do {} while(false)
#else
#define assert(expr) assert_se(expr)
#endif
I didn't check for any external libraries systemd might use.
I believe it violates the principle of least surprise, which I generally would consider a good principle.
As a new reader of the code, you'd be quite sane to expect "assert" to mean assert(), not "our local assert(), which is something different".
There should be a --disallow-keyword-redefinitions flag to the preprocessor, or something. Grumble.
Also, invoking the principle of least surprise on a C codebase is just ludicrous.
I would think it just as bad an idea to overload `NULL` with something non-standard.
The standard things in the language are something to be learned, and they should remain constant and not change per-project just because someone can do it differently. Do it however you like, but don't clobber the standard names, is all I'm saying.
> I should point out that this is not theoretical. I went through all of the above because some real machines hit this for some reason. I don't have access to them, so I had to work backwards from just the message logged by init.
> […]
> Let me say this again: this happens in the wild. The "touch * * *..." repro looks contrived, because, well, that's the whole point of a repro.
Let me quote again, with a finer slicing:
> I went through all of the above because some real machines hit this for some reason. I don't have access to them
Have you ever heard of "reproducing a problem"?
But yes, it is really strange. This usually means some weird proprietary tools
Boy, do you deserve these downvotes.
1) For me, this is a prime example of why I personally like programming environments with exceptions. If libnih could throw an exception (I know it can't), then they could do that which would allow the caller to at least deal with the exception and not bring the system down. If they don't handle the exception, well, we're were we are today, but as it stands now, fixing this will require somebody to actually patch libnih.
Yes. libnih could also handle that error by returning an error code itself, but the library developers clearly didn't want to bother with that in other callers of the affected function.
By using exceptions, for the same amount of work it took to add the assertion they could also have at least provided the option for the machine to not go down.
Also, I do understand the reservations against exceptions, but stuff like this is what makes me personally prefer having exceptions over not having them.
2) I read some passive aggressive "this is what happens if your init system is too complicated" assertions between the lines.
Being able to quickly change something in /etc/init and then have the system react to that is actually very convenient (and a must if you are pid 1 and don't want to force restarts on users).
Yes, the system is not prepared to handle 10Ks of init scripts changing, but if you're root (which you have to be to trigger this), there are way more convenient (and quicker!) ways to bring down a machine (shutdown -h being one of them).
Just removing a convenient feature because of some risk that the feature could possibly be abused by an admin IMHO isn't the right thing to do.
3) I agree with not accepting the patch. You don't (ever! ever!) fix a problem by ignoring it somewhere down the stack. You also don't call exit() or an equivalent in a library either of course :-).
The correct fix would be to remove the assertion, to return an error code and to fix all call sites (good luck with the public API that you've just now changed).
Or to throw an exception which brings us back to point 1.
I'm not complaining, btw: Stuff has happened (great analysis of the issue, btw. Much appreciated as it allowed me to completely understand the issue and be able to write this comment without having to do the analysis myself). I see why and I also understand that fixing it isn't that easy. Software is complicated.
The one thing that I'm heavily disagreeing though is above point 2). Being able to just edit a file is way more convenient than also having to restart some daemon (especially if that has PID 1). The only fix from upstarts perspective would be to forego the usage of libnih (where the bug lives), but that would mean a lot of additional maintenance work in order to protect against a totally theoretical issue as this bug requires root rights to use.
The whole problem is that the library doesn't notice the error. If you don't notice an error you are not going to use an exception either.
Not to mention that an exception by default terminates the program, which is exactly what you don't want to do.
If the library was at liberty to throw, they would not have to change any internal callers and would get the same feature for them (not having to deal with errors anywhere) but they would still give the external caller a chance not to terminate.
Yes. By default, a caller would terminate, but it would get a chance not to in a way that doesn't involve changing the libraries public interface.
So the authors did not realise inotify could legitimately return an error.
If they'd thought it could return an error, they'd return the error code themselves or recover gracefully or whatever.
Even without that, callers do have a way of preventing termination: Fork.
If an exception crosses a stack frame that isn't expecting one, things could be left in an inconsistent state. Invariants may vary, constraints may be unconstrained.
To retrofit exceptions into the existing code at this point would be very difficult, as you would have to audit every caller of the function in question, a nd the callersof those, and on and on...
Imagine problems like Apple's "goto fail" SSL bug, but more obscure and harder to find.
To have used exceptions from the beginning would require an amount of work roughly equivalent to returning error codes. They aren't fundamentally different from error codes in that way. Well, unless you're a cowboy who ignores errors.
Exceptions wouldn't have solved anything here.
There are a lot of things that aren't silver bullets. Oddly though, they are still helpful.
> One can't simply turn an assert into an exception and expect things to work.
Actually, if you did that, you'd expect them to fail... but with better semantics.
> That's a great way to add new and tricky bugs unrelated to the original issue.
Only if your runtime doesn't allow for the clean expression of stack unwinding semantics.
> To retrofit exceptions into the existing code at this point would be very difficult.
Yes, I don't think that was at all related to the original point though. It'd be hard to retrofit almost any new language feature in to the existing code.
> To have used exceptions from the beginning would require an amount of work roughly equivalent to returning error codes. They aren't fundamentally different from error codes in that way. Well, unless you're a cowboy who ignores errors.
Not at all. Error codes require error handling in every caller up the call chain. Exceptions at least allow you to put your error handling logic just where you have handlers.
The point here is that if a library has already chosen to not cleanly unwind anything, your application catching the exception isn't going to help the garbled state created by the library.
It's worth noting that that argument depends on the exception-raising assertion having been present from the beginning. The correctness of stack unwinding code can sometimes depend of which exceptions a function raises. A particular piece of code that is currently correct may break if one of the functions it calls is changed to raise a new type of exception.
[optiplex /home/chris/tmp/atexit_main]
$ ./testme
This is main. I will sleep a while, then try to exit.
PANIC! main() has been restarted due to a unscheduled exit!
This is main. I will sleep a while, then try to exit.
PANIC! main() has been restarted due to a unscheduled exit!
This is main. I will sleep a while, then try to exit.
PANIC! main() has been restarted due to a unscheduled exit!
This is main. I will sleep a while, then try to exit.From my reading of the article the author saw this issue in the wild (the assert) and then developed the 1000 touch approach to demonstrate the issue.
I'm not sure I see how it's a must. Why not just have init respond to SIGHUP like so many daemons and reload its configuration then?
I see why and I also understand that fixing it isn't that easy. Software is complicated.
The flip side of this is that perhaps we should aim to reduce the complexity of software. In this case, it's not like libnih is providing any significant increase in functionality over just using inotify anyway.
Run it in a different process. Have said process signal if a change is found. If they want to handle the case where /etc/init.d is potentially huge and you don't want to rescan everything, have it write changes via a pipe.
(In fact there's a program that will do that for you: inotifywait)
This will cause the library to stall, which might be unacceptable to some people, but if simply noting the error and dropping it is unacceptable then you really don't have a choice.
I like Go's approach: no asserts, and only use exceptions for really exceptional stuff. (Go calls exceptions panic/recover, and tweaks the recover syntax in such a way that it's much less tempting to use it for normal error handling.)
From an outsiders perspective, with zero-knowledge, it seems like libnih should be propagating back the buffer-full error.
I agree that silently failing to reload config files in only certain circumstances could be just as bad or worse behavior than just crashing, but both behaviors seem really really bad in core system software, n'est pas?
The Go FAQ defends this decision by talking about error handling, but using assertions for error handling has always been wrong.
When events are dropped, init should manually scan the config files for changes until the event queue catches up again.
- A check that can be disabled at compile time - A check that blows up the program if it fails - A check that has a really convenient syntax
The "disabled at compile time" bit is pretty much insane (at least in a language with side-effects) -- you'll inevitably end up accidentally having some sort of side-effect in your asserts, and if you turn them off for your production build, you're basically inserting a whole brand-new untested configuration, running in a build that is particularly hard to debug. But probably most people just leave asserts "on" all the time. (Even if you don't have side-effects in your asserts, you're still enabling untested run-paths, which is going to give weird, hard to debug error reports.)
The second thing is blowing up the program. Sometimes, this really is all you can do, and every language has some way for this to happen -- take the head of an empty list, access beyond an array bounds, whatever.
The problem is combining it with the really convenient syntax -- it's just very tempting to assert conditions which you should actually be handling more robustly, because assert is the easiest thing to write.
I'm doing a mix of Go server-side dev and Obj-C client side dev at the moment, and I do use asserts in Obj-C land.
I try to only use them only for truly "programmer screwed up" conditions, which should all be caught during development. But we still get crashes now and then from real users from asserts that shouldn't have been asserts; we fall into temptation.
So I suspect that actually we might be better of in Obj-C land, too, avoiding asserts.
(I also like the invariant-style thinking and documentation quality of asserts, though.)
And there are languages where assertions throw a standard (i.e. catchable) exception.
2. Well, it certainly is true that more code and more functionality is likely to contain more bugs. Whether your pid 1 needs to be doing all the things that upstart does is obviously a contentious issue.
3. This is a toss-up. I usually like 'crash loudly' over 'fail silently', but I think rejecting the patch outright was a poor decision.
I agree with your other two points though.
Famously, "for loops" are basically 'goto' with a pretty wrapper.
Trivial response: for loops don't potentially involve jumping into code in a completely different file, which could have been written by someone else.
More substantive response: for loops don't imply a specific philosophy about error handling.
That said, I do like exceptions, as I think they encourage a healthy ignorance in most code, as long as the code which can handle the exception does handle it. But that's just a "don't write bad code" admonition, and you can abuse any philosophy to write bad code.
The actual solution here is Maybe / Optionals (depending on your language of choice) as Haskell and Rust and Scala have. These would require the caller to unwrap the result, and thus make this bug actually impossible. The "returns -1" really is the equivalent of a null pointer exception which is an issue that is solved by Options not by Exceptions. Exceptions are definitely not a solution in any way.
2) I didn't see the "too complicated" between the lines bit. However, you're wrong about having to be root to trigger this. The watch, if you missed it, was on all of /etc. There are many folders in /etc which are not root owned. Furthermore, just because you have to be root to cause unexpected behavior means little in this case. Sure, this won't likely be exploited maliciously to take down a box and if that were the fear it wouldn't matter. That wasn't the concern though. The concern is this happening by accident. Even if a program is running as root, it shouldn't be able to bring down the box purely by accident.
3) A patch that prevents an init system from crashing at the expense of missing a file change (which was under /etc and quite possibly not even relevant to the init system) isn't that bad. Take it and add an issue to improve it (e.g. by directory walking as suggested). Seriously, your init system crashing because a few programs decide to write a few hundred default config files to /etc, is dumb.
For an easy example: using ansible-galaxy to fetch a role/playbook, by default (at least in the past) installed in /etc/ansible/roles/$ROLENAME. These roles can be on the order of several thousand files. The init system need not care about this at all.
The most important point here, though, is that Exceptions are absolutely not the solution here and, in fact, we've known that exceptions are fundamentally wrong for the last 40 years and yet people still learn them as part of Java and think they're good design.
The bug is not fixed because in order to trigger it you need root to spam file operations in /etc/init, which implies bigger problems elsewhere. If you have root and want to see panics, just echo c >/proc/sysrq-trigger.
EDIT: perhaps not so negligible after all. See justincormack's comment.
Actually it's libnih that doesn't deal with the overflow. The difference is that any other application using that library will also abort on that condition. Let the new bug hunt begin!
Tecnically Upstart doesn't even see the overflow, libnih blows up without notifying its caller.
All software has bugs, and Upstart is no exception, so it should not be a great surprise or tragedy that a bug is found. If you could do this as a non-root user, that would be worthy of the "I just can't believe Upstart could have a bug!" type post. But this is a pretty standard this-should-never-happen-in-a-normal-system bug.
Sometimes when a bug has a very low likelihood of happening in a normal system, it gets ignored; in this case the bug was noticed, but the bug needed work, which was never done. I know rachel says it came up on a production system, but that does not mean it was operating under normal circumstances. Still, this bug should be fixed.
Not that I need to mention it, but I will because i'm a dick: This problem would not happen with sysvinit. One more reason for me to keep using Slackware...
"If you poke around in the source, you can find that it actually registers watches for the entire directory of its config file for various reasons, so it winds up following /etc (due to /etc/init.conf) and /etc/init (its "job dir")."
> I should point out that this is not theoretical. I went through all of the above because some real machines hit this for some reason. I don't have access to them, so I had to work backwards from just the message logged by init. Then I worked forwards with a successful reproduction case to get to this point. I have no idea what the original machines are doing to make this fire, but it's probably something bizarre like spamming /etc with whatever kinds of behavior will generate those inotify events libnih asked to see.
I would argue that it was almost certainly a pathological use case, as I suggested above. i.e. Something was already broken, and it triggered this crash; had it been left to its own devices, it probably would have triggered some other bad behavior eventually (disk full, OOM killer, etc., many possibilities). I don't know it, of course, since she doesn't have the details on what caused these inotify events on such a massive and rapid scale, but I'm having a hard time imagining why /etc/init would be receiving thousands of events, short of something already being broken badly.
Doing testing myself on such device on one side I follow the mantra "The art of testing is to make border cases possible and not to assume that they will not happen."
On the other side I also have to deal with safety related stuff, where there is the rule: "The safety of the system must be maintained under any circumstances including during system with failures." That it is important to maintain human safety, like a crane should work _always_ within its limits even when failed sensors provide misreadings.
That is the same here, even when a certain service is going wild, system integrity and function must be maintained. Ignoring this fact under the assumption the cause is something else is for me just general ignorance in providing quality work.
But, being said that, part of safety related development is, to cover any theoretically possible behavior. Because not doing it, leads to systematic failures which will decrease the overall system safety. Knowing this will prevent certification with according authorities, like FDA in medical equipment, LLoyds in ships, TÜV in off-road vehicles.
At the end, knowing that such bugs are just ignored with such blatant arguments fuels the image of bad software quality.
Perhaps the repro seems pathological. But fix this issue, and you may well have fixed a whole bunch of other issues that are not so pathological. Certainly, just touching Files should never force the system to reboot!
What really caught my eye is that this affected a newer init system, specifically because it was more dynamic (using inotify), which has been a goal of many init replacements. I'm curious if this affects other init systems, specifically old init or systemd. Or if you could find similar attacks against other init systems.
1. Apparently it watches all of /etc, not just /etc/init,
2. Which means that writing large-enough config changes in some part of /etc you thought was completely unrelated to init can unexpectedly take down your box.
That's not good, and not something that can be dismissed as "of course superusers can turn the box off".