4 ms·

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

by shaurz 12y ago

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

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

by 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