Diaspora-common: does 'rm -rf /' on purge | Hacker News Reader

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

bugs.debian.org

104 pointslamby69 comments

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.

To expound on this:

"set -e" is short for "set -o errexit", that is, abort the script if a command returns with a non-zero exit code.

"set -u" is short for "set -o nounset", that is, abort the script if a variable name is dereferenced when the variable hasn't been set.

I also strongly suggest reading http://www.davidpashley.com/articles/writing-robust-shell-sc... for more things that you should be aware of when writing bash scripts.

I also do this. However, literally just hours ago, Greg Wooledge, author of The Bash Guide, posted the following to the GNU bug-bash mailing list.

> Errexit (a.k.a. set -e) is horrible, and you should not be using it in any new shell scripts you write. It exists solely for support of legacy scripts.

http://lists.gnu.org/archive/html/bug-bash/2017-03/msg00170....

edit: I asked after this (for whatever reason, my email isn't yet showing in the archives; maybe it will appear later), and got an interesting response!

http://lists.gnu.org/archive/html/bug-bash/2017-03/msg00171....

> 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

> 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_...

Bash is not always /bin/bash. On some systems (BSDs) it is /usr/bin/bash.

So the portable sh'bang line for bash is:

    #!/usr/bin/env bash

And there you can't add "-eu" anymore, because only one argument is possible in sh'bang lines. (/usr/bin/env would try to find an executable named "bash -eu")

    #!/usr/bin/env bash
    set -eu

is the more portable approach. However, for "/bin/sh" this should work portably:

    #!/bin/sh -eu

> Bash is not always /bin/bash. On some systems (BSDs) it is /usr/bin/bash.

Nitpick: on every BSD system I've used, it's /usr/local/bin/bash. But I definitely agree that using /usr/bin/env bash is the preferred solution for when you want to use bash (although I don't think it's even installed by default on most BSD's); as an Arch user (where /usr/bin/python is symlinked to python3 rather than python2), I appreciate those who go out of their way to use proper, portable shebangs.

You can also do

   #!/bin/sh -eu

Although i'm not sure if there are any advantages/disadvantages

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.)

Is there a way to do this in Fish shell? So far the only thing I could find were bugs closed as duplicates that led to this open bug:

https://github.com/fish-shell/fish-shell/issues/510

I usually add `pipefail` as well.

    set -euo pipefail

pipefail is bash-specific, no?

The other one worth recommending: -o pipefail; Which makes: false | true count as an error, where otherwise it would be ignored.

Agree, but be aware of subshells, these options don't propagate there.

  $ set -eu
  $ echo "$(false)"
  
  $

    #!/bin/bash
    set -eu
    export SHELLOPTS

That doesn't solve the problem that parent incorrectly explained.

    echo "$(false)"

swallows the exit code from $(false).

    $ cat /tmp/test.sh
    set -euvx
    export SHELLOPTS
    echo "$(false)"
    echo "echo of string with failed subcommand does not kill script"
    $(false)
    echo "but consuming exit code does"
        
    $ /tmp/test.sh        
    export SHELLOPTS
    + export SHELLOPTS
    echo "$(false)"
    false
    ++ false
    + echo ''
        
    echo "echo of string with failed subcommand does not kill script"
    + echo 'echo of string with failed subcommand does not kill script'
    echo of string with failed subcommand does not kill script
    $(false)
    false
    ++ false

These are the kinds of pitfalls, aside from everything else in this thread, that make me just use Ruby for scripting instead.

I mean, I can always just backtick pipe anything that would be convenient but avoid this nonsense.

I also use set -f, which disables pathname expansion. It's trivial to enable pathname expansion around the few lines where that's desirable.

Also -C so you don't accidentally clobber existing files. Useful to prevent symlink attacks, in addition to preventing stupid stuff.

My standard non-interactive shell script preamble begins with

  set -e # strict error
  set -u # don't expand unbound variable
  set -f # disable pathname expansion
  set -C # noclobber

I start out like that, but often I end up having to remove -u if I'm e.g. trying to check if $1 is populated. Is there a way to do that while still using -u?

Same as any potentially unbound variable, by providing a (possibly empty) default value:

    if [ "${1:-}" == "my awesome value" ]; then ...

No no no. Clearly sh and bash are just broken. We need to write scripts in Rust or Haskell to avoid safety issues. </sarcasm>

Well, Rust and Haskell might not be for the job, but sh and bash are clearly broken and in many ways.

-eu should have been the default from the start, for one.

Well, for scripts. set -e on an interactive shell is a bit of a challenge.

A shell could, and I'm going out on a limb here, check whether it's interactive or not and just enforce the -e in the appropriate case...

interactive commands should abort the command on unset variable

"Abort command if variables are unset" is the -u flag, not -e. Both were mentioned long ago in this thread, but GP was talking about -e being painful for interactive shells. Imagine an interactive shell exiting as soon as a user's command failed!

> Imagine an interactive shell exiting as soon as a user's command failed!

Hardcore mode.

sh is fundamentally broken as soon as you use a pipe in a command; bash at least has "set -o pipefail".

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_...

Creating the same bug in a code tree editor would certainly not help anything.

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.)

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.

Coreutils 'rm' has a --preserve-root option (the default) which does exactly this.

https://www.gnu.org/software/coreutils/manual/html_node/Trea...

A language with more rigor about path concatenation (and undefined variables) would. (Yes set -e, but it's little used for better or worse.)

Indeed. When you write "$foo/bin", you're looking for a specific operation: get the path "bin" relative to the directory $foo. Ideally this should be expressed in such a way that even if $foo somehow acquires a null or "default" value (which hopefully requires more than just naming an undefined variable, as in bash), you get an error trying to lookup relative to null, rather than the slash silently turning into the root directory.

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.

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.

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

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.

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.

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.

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....

I've some general notes on SIGPIPE mishandling at http://www.pixelbeat.org/programming/sigpipe_handling.html

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.

It isn't that a package manager should execute code, it's that it for tasks such as "install" and "remove", that's the package manager's job, and it shouldn't execute code for that. You still need to build the package somehow, which pretty much entails executing code.

For example, Gentoo's portage is quite capable of running code, since most packages are built from source, but that's done in a separate staging area; my understanding is that the completion of this process is a set of files that the package's build code wants to install. The package manager then installs them, from which it then also learns what to remove (what files belong to that package), and if anything would conflict.

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.

This is exactly what pkg command on Solaris 11 does, though. The package manifests only are allowed to declare certain things: files, users, device drivers, and so forth. They are allowed to include service definitions as well, but services then don't need to touch anything outside of their designated area. On removal, no arbitrary code gets to run - pkg just removed the declared files, drivers, users and services.

Transitioning some packages took work and thought, but overall the system seemed pretty darn good to me.

You're essentially asking for a whole new operating system.

If the new operating system has transparent rules about resources and ownership, whether that's files or users or any other system-level entity, that sounds like a great idea.

In the typical Linux model, we build programs from source with makefiles that can do anything or we install programs from package repositories with scripts that can do anything. In many cases, we even do this while running with root privileges.

Sometimes people talk as if this is a great thing, but the reality is that it's crude, opaque, error-prone and dangerous. If someone suggested such a clumsy design as the foundation for almost everything we do in a new operating system today, they'd be laughed out of the room. But we tolerate it in Linux because there's so much already built that way and the cost of changing would be very high.

Not really. It simply means that mechanisms need to be built that enable settings changes in a OS supplied fashion. Solaris has come a long way in this with SMF's configuration utilities svccfg and svcprop. From a configuration management perspective this approach is dramatically better than scripts that try to edit text files.

Systems have already moved in that direction for years, in the form of configuration directories (ending in .d) that packages can drop configuration files into.

Doesn't Nix offer at least an attempt at this?

Nix offers a very good attempt at this. There are some exceptions at the NixOS level, but they're kept to a minimum; mutable state is largely kept to just restarting/starting/stopping systemd services when the system changes.

And /etc is not among the exceptions; it's built as a package, just like nearly everything else. If something goes catastrophically wrong, you can still roll your system back to a previous version. (Through GRUB, if it's completely broken.)

For a concrete example, /etc/passwd is generated by a single NixOS module -- that is, Nix code. That module reads a list of users provided by other modules, and makes sure to e.g. avoid any duplicates. It's impossible to switch the system into a state where two users have the same UID, so while rolling back from that would be doable, you can't get there in the first place and (nonexistent) purge code could not be run.

Nix is more than an attempt. It is a success. It has demonstrated that POLA is worthwhile for package management.

You're essentially asking for a whole new operating system.

If the new operating system has transparent rules about resources and ownership, whether that's files or users or any other system-level entity, that sounds like a great idea.

https://gobolinux.org/faq.html

Correct. This is how Solaris IPS works.

Then you need to put that maintenance code elsewhere (and invent independent mechanisms to call it). What's the benefit?

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.

How is that different than GitSrc or Quilt source formats in Debian?

Why should it be?

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

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/

[1] https://atom.io/packages/linter-shellcheck

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

shellcheck is also in the community repo: https://www.archlinux.org/packages/community/x86_64/shellche...

I'll take this opportunity to plug my most popular essay of all time here:

http://redsymbol.net/articles/unofficial-bash-strict-mode/

Looks like it might have prevented this.

And its fixed already in https://anonscm.debian.org/cgit/pkg-ruby-extras/diaspora-ins...

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.

Well, feel free to submit a patch with an explanation: you'll improve the situation, and others will learn from it!

Well, I hope, no user has space at the end of his homedir.

You hope.

Reply on news.ycombinator.com