Diaspora-common: does 'rm -rf /' on purge(bugs.debian.org)

104 pointsby lamby9 yıl önce68 comments

bugs.debian.org

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

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=858521

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

wahern9 yıl önce

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

cellularmitosis9 yıl önce

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?

mdaniel9 yıl önce

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

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

vog9 yıl önce

> 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

zwp9 yıl önce

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

vog9 yıl önce

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

So

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

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

    #!/bin/sh -eu

saghm9 yıl önce

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

shawabawa39 yıl önce

You can also do

   #!/bin/sh -eu

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

wahern9 yıl önce

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

djsumdog9 yıl önce

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

leni5369 yıl önce

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

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

teaearlgraycold9 yıl önce

    #!/bin/bash
    set -eu
    export SHELLOPTS

harryjo9 yıl önce

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

Slackwise9 yıl önce

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.

wolfgang429 yıl önce

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.

vkjv9 yıl önce

I usually add `pipefail` as well.

    set -euo pipefail

jcassee9 yıl önce

pipefail is bash-specific, no?

IgorPartola9 yıl önce

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

coldtea9 yıl önce

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.

TorKlingberg9 yıl önce

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

coldtea9 yıl önce

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

harryjo9 yıl önce

interactive commands should abort the command on unset variable

tomjakubowski9 yıl önce

"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!

dllthomas9 yıl önce

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

Hardcore mode.

the_why_of_y9 yıl önce

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

GauntletWizard9 yıl önce

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

saurik9 yıl önce

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

redsymbol9 yıl önce

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.

[deleted]9 yıl önce

octo_t9 yıl önce

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

megous9 yıl önce

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

louiz9 yıl önce

You hope.

avian9 yıl önce

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.

zeveb9 yıl önce

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

wyldfire9 yıl önce

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.

ctrlrsf9 yıl önce

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

LyndsySimon9 yıl önce

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.

a3n9 yıl önce

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.

peff9 yıl önce

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.

pixelbeat9 yıl önce

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

LyndsySimon9 yıl önce

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

megous9 yıl önce

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

thinkmoore9 yıl önce

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

tremon9 yıl önce

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.

rwmj9 yıl önce

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

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

Ericson23149 yıl önce

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

comex9 yıl önce

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.

[deleted]9 yıl önce

eliaspro9 yıl önce

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.

deathanatos9 yıl önce

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.

barrkel9 yıl önce

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.

kemitche9 yıl önce

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.

Silhouette9 yıl önce

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.

wang_li9 yıl önce

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.

JoshTriplett9 yıl önce

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.

[deleted]9 yıl önce

alkonaut9 yıl önce

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

Filligree9 yıl önce

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.

BuuQu9hu9 yıl önce

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

Silhouette9 yıl önce

jlebrech9 yıl önce

https://gobolinux.org/faq.html

4ad9 yıl önce

Correct. This is how Solaris IPS works.

jstimpfle9 yıl önce

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

megous9 yıl önce

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.

mkhpalm9 yıl önce

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

megous9 yıl önce

Why should it be?

tveita9 yıl önce

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

kenny_r9 yıl önce

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

lttlrck9 yıl önce

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

andreyv9 yıl önce

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

makecheck9 yıl önce

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.