8 ms·

Diaspora-common: does 'rm -rf /' on purge

104 points by lamby 10y ago

octo_t 10y ago
And its fixed already in https://anonscm.debian.org/cgit/pkg-ruby-extras/diaspora-installer.git/commit/?id=7071a253e8404e41e7767b4330899a2da045731c https://anonscm.debian.org/cgit/pkg-ruby-extras/diaspora-ins...
- megous 10y ago
  Well, I hope, no user has space at the end of his homedir.
  - louiz 10y ago
    You hope.
- avian 10y ago
  This is not a particularly good fix. It lacks "set -u" which is the first line of defense for this kind of bugs, much more robust than the "if [ -d ...". Command lines lack proper shell quoting. If $diaspora_home ends up with a trailing space somehow, this will again end in a disaster. I don't see the point of adding a "test -e". It seems "rm -rf /var/cache/diaspora /var/log/diaspora" got lost, so these directories are no longer cleaned up after uninstall.
  - zeveb 10y ago
    Well, feel free to submit a patch with an explanation: you'll improve the situation, and others will learn from it!
wyldfire 10y ago
Sure, this is a good idea (from the proposed patch): + # safety check + [ "${diaspora_user_home}" != "" ] || exit 1 + [ "${diaspora_home}" != "" ] || exit 1 ... but this is key IMO: +# Abort if any unbound variables are used +#set -u I often use "set -euo pipefail" in my bash scripts. It does come with some caveats, though.
- LyndsySimon 10y ago
  To be fair, the proposed patch says on the box that it's "completely untested". The author seems to be politely saying that the package is a hot mess, and my impression was that they weren't willing to put any more time into it.
  - a3n 10y ago
    My impression was that the submission of the untested patch ("testing welcome") was that it was a fast first step in collaboratively fixing the issue.
- ctrlrsf 10y ago
  I use the long form so others reading script that may not be familiar with set options gets a hint of what's going on. At the top of each file with a brief comment I add: set -o errexit set -o nounset set -o pipefail
- peff 10y ago
  Using "set -e" with pipefail is racy because of SIGPIPE. For example, if you do this: some_command | head Then depending on timing, `some_command` _may_ generate all of its output and stuff it into the pipe, exiting successfully. Or `head` may get the lines it needs and close the pipe, causing `some_command` to exit with SIGPIPE. Using pipefail makes the whole pipeline fail, and then `-e` terminates the script. And it only happens sometimes.
  - pixelbeat 10y ago
    That really is a bash bug IMHO I've discussed this with upstream bash at http://lists.gnu.org/archive/html/bug-bash/2015-02/msg00052.html http://lists.gnu.org/archive/html/bug-bash/2015-02/msg00052.... I've some general notes on SIGPIPE mishandling at http://www.pixelbeat.org/programming/sigpipe_handling.html http://www.pixelbeat.org/programming/sigpipe_handling.html
LyndsySimon 10y ago
From reddit: > It's actually rm -rf /bin. > Seems to be caused by the same reason as the famous Steam bug - using an undefined variable. Plus the default bash behavior that simply ignores the fact that it doesn't exist and treats it like an empty string. > tl;dr flamebait: The Unix way™ of "everything is just text" strikes again. (EDIT: yes this a gross over-generalization) source: https://www.reddit.com/r/programming/comments/610z2f/858521_diasporacommon_does_rm_rf_on_purge/dfauurn/ https://www.reddit.com/r/programming/comments/610z2f/858521_...
- megous 10y ago
  Creating the same bug in a code tree editor would certainly not help anything.
  - thinkmoore 10y ago
    I think the alternative being suggested is that rm shouldn't accept a string that magically can reference anything in the filesystem... In our capability-based shell scripting language Shill (shill-lang.org) for example, you could prevent this sort of bug by giving the script a capability for just the diaspora_home directory, and deriving the child directories from that capability. (Of course, you still need to make sure you pass in the right directory in the first place.)
    - tremon 10y ago
      I think the alternative being suggested is that rm shouldn't accept a string that magically can reference anything in the filesystem... That suggestion fails to grasp the basics of Unix shells however: parameter expansion as well as globbing is performed by the shell, rm "sees" nothing but fully-expanded paths.
    - rwmj 10y ago
      Coreutils 'rm' has a --preserve-root option (the default) which does exactly this. https://www.gnu.org/software/coreutils/manual/html_node/Treating-_002f-specially.html https://www.gnu.org/software/coreutils/manual/html_node/Trea...
eliaspro 10y ago
I still think a package manager should never execute any code at all. It should extract a package and remove its files at some point again, but that's it.
- jstimpfle 10y ago
  Then you need to put that maintenance code elsewhere (and invent independent mechanisms to call it). What's the benefit?
  - megous 10y ago
    That's where Arch Linux (pacman) is heading slowly. The code is better tested that way, and not written multiple times by many programmers/packagers of various skill. Also, it's the difference between systemd and sysvinit - descriptive configuration vs scripting.
    - mkhpalm 10y ago
      How is that different than GitSrc or Quilt source formats in Debian?
      - megous 10y ago
        Why should it be?
- barrkel 10y ago
  That would require a reorientation of the whole distribution around the requirement that packages integrate only at the level of whole files. Everything from interacting services to user and password management, from drivers to application plugins, all of it would need to be integrated solely using atomic files on disk. Anything that couldn't be packaged that way would need to be rewritten or forked. You're essentially asking for a whole new operating system.
  - wang_li 10y ago
tveita 10y ago
Looks like that script could benefit from a couple of rounds of ShellCheck. http://www.shellcheck.net/ http://www.shellcheck.net/
- kenny_r 10y ago
  ShellCheck warned me of a very similar issue in a script of mine recently. In that script I ran the following: rm -rf "${FOO}/" ShellCheck helpfully warned me that that could have disastrous consequences when FOO is unset, instead it advised me to use this: rm -rf "${FOO:?}/" I'd also like to point out that ShellCheck is in the AUR for users of Arch Linux[0], and that there's a handy plugin which I use for the Atom editor[1]. [0] https://aur.archlinux.org/packages/shellcheck-git/ https://aur.archlinux.org/packages/shellcheck-git/ [1] https://atom.io/packages/linter-shellcheck https://atom.io/packages/linter-shellcheck
  - andreyv 10y ago
    shellcheck is also in the community repo: https://www.archlinux.org/packages/community/x86_64/shellcheck/ https://www.archlinux.org/packages/community/x86_64/shellche...
  - lttlrck 10y ago
    There is also a plugin for sublime text [0] It doesn't take long to internalize the corrections. It's well worth using a linter if one is available. [0] https://github.com/SublimeLinter/SublimeLinter-shellcheck https://github.com/SublimeLinter/SublimeLinter-shellcheck
makecheck 10y ago
Wildcards and variables in “risky” commands are a way of favoring convenience over safety and auditability, favoring writing (done rarely) over reading (more frequent). They are also unlikely to stand the test of time because the meaning can change, casting a different net than you originally intended. If a command is risky, you should at least GENERATE the variable-expanded list of inputs (e.g. in a separate file) so that the list can be audited. For instance, basic automatic sanity checks can be performed on the expansion like “length > 1”, and you can see every affected item. It also acts as a log of impacted things in case it does run. Robust code is not always convenient.
deleted 10y ago
[deleted]
theaustinseven 10y ago
All scripts should be run with "-eu". This prevents unbound variables from ever being used and will end the script on a failed command. If you want commands in the script to fail, just drop the "e" and run with "-u". At my last job we put "-eu" in literally all of our scripts to avoid this very problem.
- vog 10y ago
  > All scripts should be run with "-eu". More specifically, every shell script should have "set -eu" at its second line, directly after the sh'bang line: #!... set -eu For debugging or logging purposes, sometimes adding -v and/or -x comes handy: #!... set -euvx
  - shawabawa3 10y ago
    You can also do #!/bin/sh -eu Although i'm not sure if there are any advantages/disadvantages
    - wahern 10y ago
      Disadvantage is that nobody can use an alternative shell to run your script as intended. (Alternative here could mean a less buggy version of whatever is the system shell.)
  - zwp 10y ago
    > should have "set -eu" at its second line Do you have a strong reason to prefer "set" over shebang flags? I have a slight preference for shebang flags so I can deliberately override them from the command line (but it's not a hill I'd die on): $ cat foo #!/bin/bash -eu cd /nowhere echo 'still here' $ ./foo ./foo: line 2: cd: nowhere: No such file or directory $ bash -c ./foo # thinking about system(3) ./foo: line 2: cd: nowhere: No such file or directory $ bash +e ./foo # override ./foo: line 2: cd: nowhere: No such file or directory still here $ EDIT: Google's shell style guide has "Executables must start with #!/bin/bash and a minimum number of flags. Use set to set shell options so that calling your script as bash <script_name> does not break its functionality." https://google.github.io/styleguide/shell.xml?showone=Which_Shell_to_Use#Which_Shell_to_Use https://google.github.io/styleguide/shell.xml?showone=Which_...
deleted 10y ago
[deleted]
redsymbol 10y ago
I'll take this opportunity to plug my most popular essay of all time here: http://redsymbol.net/articles/unofficial-bash-strict-mode/ http://redsymbol.net/articles/unofficial-bash-strict-mode/ Looks like it might have prevented this.

104 points by lamby 10y ago

And its fixed already in https://anonscm.debian.org/cgit/pkg-ruby-extras/diaspora-installer.git/commit/?id=7071a253e8404e41e7767b4330899a2da045731c https://anonscm.debian.org/cgit/pkg-ruby-extras/diaspora-ins...

megous 10y ago
Well, I hope, no user has space at the end of his homedir.
- louiz 10y ago
  You hope.
avian 10y ago
This is not a particularly good fix. It lacks "set -u" which is the first line of defense for this kind of bugs, much more robust than the "if [ -d ...". Command lines lack proper shell quoting. If $diaspora_home ends up with a trailing space somehow, this will again end in a disaster. I don't see the point of adding a "test -e". It seems "rm -rf /var/cache/diaspora /var/log/diaspora" got lost, so these directories are no longer cleaned up after uninstall.
- zeveb 10y ago
  Well, feel free to submit a patch with an explanation: you'll improve the situation, and others will learn from it!

wyldfire 10y ago

Sure, this is a good idea (from the proposed patch): + # safety check + [ "${diaspora_user_home}" != "" ] || exit 1 + [ "${diaspora_home}" != "" ] || exit 1 ... but this is key IMO: +# Abort if any unbound variables are used +#set -u I often use "set -euo pipefail" in my bash scripts. It does come with some caveats, though.

LyndsySimon 10y ago
To be fair, the proposed patch says on the box that it's "completely untested". The author seems to be politely saying that the package is a hot mess, and my impression was that they weren't willing to put any more time into it.
- a3n 10y ago
  My impression was that the submission of the untested patch ("testing welcome") was that it was a fast first step in collaboratively fixing the issue.
ctrlrsf 10y ago
I use the long form so others reading script that may not be familiar with set options gets a hint of what's going on. At the top of each file with a brief comment I add: set -o errexit set -o nounset set -o pipefail
peff 10y ago
Using "set -e" with pipefail is racy because of SIGPIPE. For example, if you do this: some_command | head Then depending on timing, `some_command` _may_ generate all of its output and stuff it into the pipe, exiting successfully. Or `head` may get the lines it needs and close the pipe, causing `some_command` to exit with SIGPIPE. Using pipefail makes the whole pipeline fail, and then `-e` terminates the script. And it only happens sometimes.
- pixelbeat 10y ago
  That really is a bash bug IMHO I've discussed this with upstream bash at http://lists.gnu.org/archive/html/bug-bash/2015-02/msg00052.html http://lists.gnu.org/archive/html/bug-bash/2015-02/msg00052.... I've some general notes on SIGPIPE mishandling at http://www.pixelbeat.org/programming/sigpipe_handling.html http://www.pixelbeat.org/programming/sigpipe_handling.html

LyndsySimon 10y ago

From reddit: > It's actually rm -rf /bin. > Seems to be caused by the same reason as the famous Steam bug - using an undefined variable. Plus the default bash behavior that simply ignores the fact that it doesn't exist and treats it like an empty string. > tl;dr flamebait: The Unix way™ of "everything is just text" strikes again. (EDIT: yes this a gross over-generalization) source: https://www.reddit.com/r/programming/comments/610z2f/858521_diasporacommon_does_rm_rf_on_purge/dfauurn/ https://www.reddit.com/r/programming/comments/610z2f/858521_...

megous 10y ago
Creating the same bug in a code tree editor would certainly not help anything.
- thinkmoore 10y ago
  I think the alternative being suggested is that rm shouldn't accept a string that magically can reference anything in the filesystem... In our capability-based shell scripting language Shill (shill-lang.org) for example, you could prevent this sort of bug by giving the script a capability for just the diaspora_home directory, and deriving the child directories from that capability. (Of course, you still need to make sure you pass in the right directory in the first place.)
  - tremon 10y ago
    I think the alternative being suggested is that rm shouldn't accept a string that magically can reference anything in the filesystem... That suggestion fails to grasp the basics of Unix shells however: parameter expansion as well as globbing is performed by the shell, rm "sees" nothing but fully-expanded paths.
  - rwmj 10y ago
    Coreutils 'rm' has a --preserve-root option (the default) which does exactly this. https://www.gnu.org/software/coreutils/manual/html_node/Treating-_002f-specially.html https://www.gnu.org/software/coreutils/manual/html_node/Trea...

eliaspro 10y ago

I still think a package manager should never execute any code at all. It should extract a package and remove its files at some point again, but that's it.

jstimpfle 10y ago
Then you need to put that maintenance code elsewhere (and invent independent mechanisms to call it). What's the benefit?
- megous 10y ago
  That's where Arch Linux (pacman) is heading slowly. The code is better tested that way, and not written multiple times by many programmers/packagers of various skill. Also, it's the difference between systemd and sysvinit - descriptive configuration vs scripting.
  - mkhpalm 10y ago
    How is that different than GitSrc or Quilt source formats in Debian?
    - megous 10y ago
      Why should it be?
barrkel 10y ago
That would require a reorientation of the whole distribution around the requirement that packages integrate only at the level of whole files. Everything from interacting services to user and password management, from drivers to application plugins, all of it would need to be integrated solely using atomic files on disk. Anything that couldn't be packaged that way would need to be rewritten or forked. You're essentially asking for a whole new operating system.
- wang_li 10y ago

tveita 10y ago

Looks like that script could benefit from a couple of rounds of ShellCheck. http://www.shellcheck.net/ http://www.shellcheck.net/

kenny_r 10y ago
ShellCheck warned me of a very similar issue in a script of mine recently. In that script I ran the following: rm -rf "${FOO}/" ShellCheck helpfully warned me that that could have disastrous consequences when FOO is unset, instead it advised me to use this: rm -rf "${FOO:?}/" I'd also like to point out that ShellCheck is in the AUR for users of Arch Linux[0], and that there's a handy plugin which I use for the Atom editor[1]. [0] https://aur.archlinux.org/packages/shellcheck-git/ https://aur.archlinux.org/packages/shellcheck-git/ [1] https://atom.io/packages/linter-shellcheck https://atom.io/packages/linter-shellcheck
- andreyv 10y ago
  shellcheck is also in the community repo: https://www.archlinux.org/packages/community/x86_64/shellcheck/ https://www.archlinux.org/packages/community/x86_64/shellche...
- lttlrck 10y ago
  There is also a plugin for sublime text [0] It doesn't take long to internalize the corrections. It's well worth using a linter if one is available. [0] https://github.com/SublimeLinter/SublimeLinter-shellcheck https://github.com/SublimeLinter/SublimeLinter-shellcheck

makecheck 10y ago

Wildcards and variables in “risky” commands are a way of favoring convenience over safety and auditability, favoring writing (done rarely) over reading (more frequent). They are also unlikely to stand the test of time because the meaning can change, casting a different net than you originally intended. If a command is risky, you should at least GENERATE the variable-expanded list of inputs (e.g. in a separate file) so that the list can be audited. For instance, basic automatic sanity checks can be performed on the expansion like “length > 1”, and you can see every affected item. It also acts as a log of impacted things in case it does run. Robust code is not always convenient.

deleted 10y ago

[deleted]

theaustinseven 10y ago

All scripts should be run with "-eu". This prevents unbound variables from ever being used and will end the script on a failed command. If you want commands in the script to fail, just drop the "e" and run with "-u". At my last job we put "-eu" in literally all of our scripts to avoid this very problem.

vog 10y ago
> All scripts should be run with "-eu". More specifically, every shell script should have "set -eu" at its second line, directly after the sh'bang line: #!... set -eu For debugging or logging purposes, sometimes adding -v and/or -x comes handy: #!... set -euvx
- shawabawa3 10y ago
  You can also do #!/bin/sh -eu Although i'm not sure if there are any advantages/disadvantages
  - wahern 10y ago
    Disadvantage is that nobody can use an alternative shell to run your script as intended. (Alternative here could mean a less buggy version of whatever is the system shell.)
- zwp 10y ago
  > should have "set -eu" at its second line Do you have a strong reason to prefer "set" over shebang flags? I have a slight preference for shebang flags so I can deliberately override them from the command line (but it's not a hill I'd die on): $ cat foo #!/bin/bash -eu cd /nowhere echo 'still here' $ ./foo ./foo: line 2: cd: nowhere: No such file or directory $ bash -c ./foo # thinking about system(3) ./foo: line 2: cd: nowhere: No such file or directory $ bash +e ./foo # override ./foo: line 2: cd: nowhere: No such file or directory still here $ EDIT: Google's shell style guide has "Executables must start with #!/bin/bash and a minimum number of flags. Use set to set shell options so that calling your script as bash <script_name> does not break its functionality." https://google.github.io/styleguide/shell.xml?showone=Which_Shell_to_Use#Which_Shell_to_Use https://google.github.io/styleguide/shell.xml?showone=Which_...

deleted 10y ago

[deleted]

redsymbol 10y ago

I'll take this opportunity to plug my most popular essay of all time here: http://redsymbol.net/articles/unofficial-bash-strict-mode/ http://redsymbol.net/articles/unofficial-bash-strict-mode/ Looks like it might have prevented this.