13 ms·

Kill init by touching a bunch of files

255 points by omnibrain 12y ago

shaurz 12y ago
It's pretty dumb that pid 1 has any assert()s in it at all. Or libraries for that matter.
- erpellan 12y ago
  You're going to _love_ systemd. I think someone found a kitchen sink in there once.
  - SwellJoe 12y ago
    I often need to wash up after particularly messy system administration tasks. That sink will come in handy. 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?
    - tankenmate 12y ago
      And then take this anti-pattern and multiply it by the sheer excess of feeping creaturism; a massive vulnerability surface. If you thought AWS's short notice reboot everything incident was/is uncommon I suspect we'll be seeing a lot more of that sort of thing in the nearish future.
    - vidarh 12y ago
      Eyeballing their inotify use, it does seem to be careful to check this field and log failures if it's not as expected, but doesn't appear to do anything that should intentionally or unintentionally trigger failure.
    - andrewaylett 12y ago
      TTBOMK, systemd doesn't watch unit files for changes. If you change a unit, you need to run `systemctl daemon-reload` to make it notice. It does, however, try to work out if it's in a state where you are running `systemctl` without reloading, and warns that you may need to reload.
  - viraptor 12y ago
    Well... on one hand side, it's probably better to abort than to get a possible exploit. On the other it's not a funny situation when one of your base services dies and it can happen easily (found some cases of remote-triggerable abort()s in systemd-resolve - patches were sent upsteam, nobody seems to care for a few days now).
    - chris_wot 12y ago
      Exploits include denial of service, surely?
  - __david__ 12y ago
    I decided to see if there was any merit to your snarky comment: $ git clone git://anongit.freedesktop.org/systemd/systemd systemd-git $ cd systemd-git $ grep -r "assert(" | wc -l 7771 That looks pretty bad at first. But I noticed this in src/shared/macro.h: /* 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 verify that macro.h was included by every file that used assert(). But I'm assuming it does. So it looks like assert() at least doesn't call abort() in their world. In fact, it's a nop in non-debug builds. And even in debug builds, the assert_se() macro logs a message and continues. I didn't check for any external libraries systemd might use.
    - vidarh 12y ago
      Also, the systemd repository contains dozens of other programs than systemd itself. Even if any of those were to abort, the implications for systemd would be far less severe. Not nearly all the git code will ever be running in pid 1.
    - unwind 12y ago
      This would be a great place to insert a snarky comment about the sanity of overloading a standard function. 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.
      - GFK_of_xmaspast 12y ago
        The "principle of least surprise" would say that of course asserts are turned off with -NDEBUG. From assert(3) on osx: """The assert() macro may be removed at compile time with the cc(1) option -DNDEBUG."""
      - tinco 12y ago
        'assert' is not a keyword. Even if it were, they overrode it with a function that has the exact same signature and general runtime behaviour. Hardly anything to be surprised about. Also, invoking the principle of least surprise on a C codebase is just ludicrous.
  - vezzy-fnord 12y ago
    Reportedly it's possible to achieve a similar effect on systemd by having too many journal files (on 208, but the distros backport patches into it constantly from the -stable branch, IIRC): https://bugzilla.redhat.com/show_bug.cgi?id=1112556 https://bugzilla.redhat.com/show_bug.cgi?id=1112556
rikkus 12y ago
.NET's FileSystemWatcher documentation says that there's an internal buffer of a finite size, and that if (when) it fills up, you're going to be told about it and must do a complete traversal of the directory you're watching. No-one has invented a better way to deal with this, so that's what you need to do. 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).
- justincormack 12y ago
  As far as I remember all the Linux kernel to userspace notifications eg netlink as well behave like this. Of course it is a pain to do and hard to test so few people do but if you are writing a library you clearly should especially if it is being run by init.
- MaulingMonkey 12y ago
  I'd generally take a config file failing to update (this may happen anyways from the looks of it, e.g. if "/proc/sys/fs/inotify/max_queued_events" isn't available) over the box dying with a terrible crash report. 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.
  - Morgawr 12y ago
    >I'd generally take a config file failing to update (this may happen anyways from the looks of it, e.g. if "/proc/sys/fs/inotify/max_queued_events" isn't available) over the box dying with a terrible crash report. 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.
SwellJoe 12y ago
It's worth noting that the root user has any number of pathological use cases that can bring down the system. This is but one of them. Interesting, but not particularly dangerous or likely to be triggered in any normal circumstance.
- scintill76 12y ago
  The post said it happened in real usage, and was then reproduced by this contrived example. Whatever the original use case was, may have still been reckless or absurd, but I think it's also worth noting that it was not originally found by intentionally throwing pathological usecases at the system.
- PinguTS 12y ago
  Maybe next time reading the full post before commenting: > 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.
  - SwellJoe 12y ago
    She doesn't know why it happened. 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.
    - justincormack 12y ago
xfs 12y ago
TL;DR: He overflowed inotify queue of /etc/init which is the Upstart configuration directory being monitored. Upstart doesn't deal with the overflow, exits, and causes kernel panic. 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.
- justincormack 12y ago
  Rachel is a she I think you will find. A better TLDR is that init can die due to valid response from the kernel. She found this in a production use case. Perhaps involving fast changing services.
- chias 12y ago
  Yeah, while the digging in "step 5" is interesting, I think the reach of this bug is fairly negligible: "root user can break the system". EDIT: perhaps not so negligible after all. See justincormack's comment.
- viraptor 12y ago
  > Upstart doesn't deal with the overflow, 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!
- masklinn 12y ago
  > Upstart doesn't deal with the overflow Tecnically Upstart doesn't even see the overflow, libnih blows up without notifying its caller.
- peterwwillis 12y ago
  Why is this getting downvoted? This is an important aspect to the bug. 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...
Havvy 12y ago
"libnih" - I have no clue what this does, so I immediately read it as 'lib not invented here'. Otherwise, yay...more unstable software.
- masklinn 12y ago
  > so I immediately read it as 'lib not invented here'. > libnih is a small library for C application development containing functions that, despite its name, are not implemented elsewhere in the standard library set.
  - unwind 12y ago
    It's a pretty fun name. :) Here's the core code, for the curious: http://bazaar.launchpad.net/~scott/libnih/trunk/files/head:/nih/ 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.
angry_octet 12y ago
This is a great example of how bad many open source projects are at accepting contributions from 'non core' developers. The patch is just rejected, when it actually looks pretty valid to handle all cases of return value from a kernel interface. While it might not be a perfect solution, accepting it with suggestions for additional improvements could have led to those improvements.
- x0x0 12y ago
  I came here to say, well, something. 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.
- masklinn 12y ago
  > The patch is just rejected 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/+merge/140150 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.
  - marcosdumay 12y ago
    The OS disobeying your configuration files is really better than it restarting? Looks to me that Upstart kept the lesser of two evils. I'd reject that patch too.
    - masklinn 12y ago
      > The OS disobeying your configuration files is really better than it restarting? 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.
nodata 12y ago
Writing to /etc/init.d requires root access. If you are root, you can bring down the box as it is.
- ge0rg 12y ago
  The problem here was that the box went down by accident due to this bug.
  - nodata 12y ago
    Really? How?
    - masklinn 12y ago
      Because the bug kills pid1 which took down production boxes: > 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.
      - nodata 12y ago
        No, how did you accidentally write to /etc/init.d?
        masklinn 12y ago
        > No, how did you accidentally write to /etc/init.d? 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
        nodata 12y ago
        So you don't know? Gotcha.
pilif 12y ago
There are so many things in here that tempt me to comment about, so here goes: 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.
emeidi 12y ago
As far as I understand, you need to be root (or another privileged user) who has write access to /etc/init. Conclusion: You can bring down a machine with superuser privileges. Breaking.
- sp332 12y ago
  You can set this up as an unprivileged user in any directory you have access to. Just use inotifywait to configure your own inotify triggers first.
- npsimons 12y ago
  Yeah, this reminds me of a similar article from the same source (IIRC) about "issues" with disabling the oom-killer and replacing it with your own. Interesting, probably something that needs to be addressed, but not necessarily a game changer as the article would have you believe. 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.
- ubernostrum 12y ago
  Except: 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".
- serve_yay 12y ago
  Boy oh boy the responses to this are frustrating. (And we wonder why everything is broken and shitty.)
twic 12y ago
Perhaps it would be a good move to extract the file-watching behaviour from the part of Upstart that runs as PID 1, and put it in a separate daemon that can notify Upstart when it's time to reload a configuration file. That way, only the daemon would crash in this situation, and Upstart could restart it as it would any other daemon. Although then you have the problem of the notification channel between the daemon and Upstart overflowing, i suppose.
weissadam 12y ago
The good news is that Upstart is in the process of being phased out. The bad news is its replacement. : )
uint32 12y ago
What's the point of watching for init config changes? Who has init configs that change so often that this is useful?

255 points by omnibrain 12y ago

shaurz 12y ago

It's pretty dumb that pid 1 has any assert()s in it at all. Or libraries for that matter.

erpellan 12y ago
You're going to _love_ systemd. I think someone found a kitchen sink in there once.
- SwellJoe 12y ago
  I often need to wash up after particularly messy system administration tasks. That sink will come in handy. 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?
  - tankenmate 12y ago
    And then take this anti-pattern and multiply it by the sheer excess of feeping creaturism; a massive vulnerability surface. If you thought AWS's short notice reboot everything incident was/is uncommon I suspect we'll be seeing a lot more of that sort of thing in the nearish future.
  - vidarh 12y ago
    Eyeballing their inotify use, it does seem to be careful to check this field and log failures if it's not as expected, but doesn't appear to do anything that should intentionally or unintentionally trigger failure.
  - andrewaylett 12y ago
    TTBOMK, systemd doesn't watch unit files for changes. If you change a unit, you need to run `systemctl daemon-reload` to make it notice. It does, however, try to work out if it's in a state where you are running `systemctl` without reloading, and warns that you may need to reload.
- viraptor 12y ago
  Well... on one hand side, it's probably better to abort than to get a possible exploit. On the other it's not a funny situation when one of your base services dies and it can happen easily (found some cases of remote-triggerable abort()s in systemd-resolve - patches were sent upsteam, nobody seems to care for a few days now).
  - chris_wot 12y ago
    Exploits include denial of service, surely?
- __david__ 12y ago
  I decided to see if there was any merit to your snarky comment: $ git clone git://anongit.freedesktop.org/systemd/systemd systemd-git $ cd systemd-git $ grep -r "assert(" | wc -l 7771 That looks pretty bad at first. But I noticed this in src/shared/macro.h: /* 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 verify that macro.h was included by every file that used assert(). But I'm assuming it does. So it looks like assert() at least doesn't call abort() in their world. In fact, it's a nop in non-debug builds. And even in debug builds, the assert_se() macro logs a message and continues. I didn't check for any external libraries systemd might use.
  - vidarh 12y ago
    Also, the systemd repository contains dozens of other programs than systemd itself. Even if any of those were to abort, the implications for systemd would be far less severe. Not nearly all the git code will ever be running in pid 1.
  - unwind 12y ago
    This would be a great place to insert a snarky comment about the sanity of overloading a standard function. 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.
    - GFK_of_xmaspast 12y ago
      The "principle of least surprise" would say that of course asserts are turned off with -NDEBUG. From assert(3) on osx: """The assert() macro may be removed at compile time with the cc(1) option -DNDEBUG."""
    - tinco 12y ago
      'assert' is not a keyword. Even if it were, they overrode it with a function that has the exact same signature and general runtime behaviour. Hardly anything to be surprised about. Also, invoking the principle of least surprise on a C codebase is just ludicrous.
- vezzy-fnord 12y ago
  Reportedly it's possible to achieve a similar effect on systemd by having too many journal files (on 208, but the distros backport patches into it constantly from the -stable branch, IIRC): https://bugzilla.redhat.com/show_bug.cgi?id=1112556 https://bugzilla.redhat.com/show_bug.cgi?id=1112556

rikkus 12y ago

.NET's FileSystemWatcher documentation says that there's an internal buffer of a finite size, and that if (when) it fills up, you're going to be told about it and must do a complete traversal of the directory you're watching. No-one has invented a better way to deal with this, so that's what you need to do. 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).

justincormack 12y ago
As far as I remember all the Linux kernel to userspace notifications eg netlink as well behave like this. Of course it is a pain to do and hard to test so few people do but if you are writing a library you clearly should especially if it is being run by init.
MaulingMonkey 12y ago
I'd generally take a config file failing to update (this may happen anyways from the looks of it, e.g. if "/proc/sys/fs/inotify/max_queued_events" isn't available) over the box dying with a terrible crash report. 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.
- Morgawr 12y ago
  >I'd generally take a config file failing to update (this may happen anyways from the looks of it, e.g. if "/proc/sys/fs/inotify/max_queued_events" isn't available) over the box dying with a terrible crash report. 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.

SwellJoe 12y ago

It's worth noting that the root user has any number of pathological use cases that can bring down the system. This is but one of them. Interesting, but not particularly dangerous or likely to be triggered in any normal circumstance.

scintill76 12y ago
The post said it happened in real usage, and was then reproduced by this contrived example. Whatever the original use case was, may have still been reckless or absurd, but I think it's also worth noting that it was not originally found by intentionally throwing pathological usecases at the system.
PinguTS 12y ago
Maybe next time reading the full post before commenting: > 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.
- SwellJoe 12y ago
  She doesn't know why it happened. 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.
  - justincormack 12y ago

xfs 12y ago

TL;DR: He overflowed inotify queue of /etc/init which is the Upstart configuration directory being monitored. Upstart doesn't deal with the overflow, exits, and causes kernel panic. 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.

justincormack 12y ago
Rachel is a she I think you will find. A better TLDR is that init can die due to valid response from the kernel. She found this in a production use case. Perhaps involving fast changing services.
chias 12y ago
Yeah, while the digging in "step 5" is interesting, I think the reach of this bug is fairly negligible: "root user can break the system". EDIT: perhaps not so negligible after all. See justincormack's comment.
viraptor 12y ago
> Upstart doesn't deal with the overflow, 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!
masklinn 12y ago
> Upstart doesn't deal with the overflow Tecnically Upstart doesn't even see the overflow, libnih blows up without notifying its caller.
peterwwillis 12y ago
Why is this getting downvoted? This is an important aspect to the bug. 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...

Havvy 12y ago

"libnih" - I have no clue what this does, so I immediately read it as 'lib not invented here'. Otherwise, yay...more unstable software.

masklinn 12y ago
> so I immediately read it as 'lib not invented here'. > libnih is a small library for C application development containing functions that, despite its name, are not implemented elsewhere in the standard library set.
- unwind 12y ago
  It's a pretty fun name. :) Here's the core code, for the curious: http://bazaar.launchpad.net/~scott/libnih/trunk/files/head:/nih/ 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.

angry_octet 12y ago

This is a great example of how bad many open source projects are at accepting contributions from 'non core' developers. The patch is just rejected, when it actually looks pretty valid to handle all cases of return value from a kernel interface. While it might not be a perfect solution, accepting it with suggestions for additional improvements could have led to those improvements.

x0x0 12y ago
I came here to say, well, something. 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.
masklinn 12y ago
> The patch is just rejected 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/+merge/140150 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.
- marcosdumay 12y ago
  The OS disobeying your configuration files is really better than it restarting? Looks to me that Upstart kept the lesser of two evils. I'd reject that patch too.
  - masklinn 12y ago
    > The OS disobeying your configuration files is really better than it restarting? 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.

nodata 12y ago

Writing to /etc/init.d requires root access. If you are root, you can bring down the box as it is.

ge0rg 12y ago
The problem here was that the box went down by accident due to this bug.
- nodata 12y ago
  Really? How?
  - masklinn 12y ago
    Because the bug kills pid1 which took down production boxes: > 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.
    - nodata 12y ago
      No, how did you accidentally write to /etc/init.d?
      - masklinn 12y ago
        > No, how did you accidentally write to /etc/init.d? 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
        nodata 12y ago
        So you don't know? Gotcha.

pilif 12y ago

There are so many things in here that tempt me to comment about, so here goes: 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.

emeidi 12y ago

As far as I understand, you need to be root (or another privileged user) who has write access to /etc/init. Conclusion: You can bring down a machine with superuser privileges. Breaking.

sp332 12y ago
You can set this up as an unprivileged user in any directory you have access to. Just use inotifywait to configure your own inotify triggers first.
npsimons 12y ago
Yeah, this reminds me of a similar article from the same source (IIRC) about "issues" with disabling the oom-killer and replacing it with your own. Interesting, probably something that needs to be addressed, but not necessarily a game changer as the article would have you believe. 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.
ubernostrum 12y ago
Except: 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".
serve_yay 12y ago
Boy oh boy the responses to this are frustrating. (And we wonder why everything is broken and shitty.)

twic 12y ago

Perhaps it would be a good move to extract the file-watching behaviour from the part of Upstart that runs as PID 1, and put it in a separate daemon that can notify Upstart when it's time to reload a configuration file. That way, only the daemon would crash in this situation, and Upstart could restart it as it would any other daemon. Although then you have the problem of the notification channel between the daemon and Upstart overflowing, i suppose.

weissadam 12y ago

The good news is that Upstart is in the process of being phased out. The bad news is its replacement. : )

uint32 12y ago

What's the point of watching for init config changes? Who has init configs that change so often that this is useful?