
Bug 1202858 – Restarting squid results in deleting all files in hard-drive - geoffbp
https://bugzilla.redhat.com/show_bug.cgi?id=1202858
======
kevan
I love how deadpan the bug report is:

    
    
        Actual results:
        All files are deleted on the machine.
    
        Expected results:
        Squid is restarted.
    

Not many details yet but it sounds similar to the Steam bug [0] from last
year.

[0] [https://github.com/valvesoftware/steam-for-
linux/issues/3671](https://github.com/valvesoftware/steam-for-
linux/issues/3671)

~~~
viraptor
I would bet on the issue being in the init script itself rather than squid.
(I'm assuming squid doesn't run as root by default in rhel) If that's true
then it's another point for more sane process managers
(upstart/supervisord/systemd/...)

~~~
kevan
Agreed, I should've elaborated. All it takes is something like this in the
init script without checking if the variable is empty:

    
    
        rm -rf "$STEAMROOT/"*

~~~
lpsz
And this is why it is important to write something like

    
    
        set -eu
    

on top of your bash scripts -- execution will stop on errors (non-zero
retvals) and on undefined variables.

~~~
jiffytick
or check the variable before using it, like any other programming language:

[[ "$VAR" ]] && rm -rf "$VAR/*"

I think most of these issues stem from the fact that most developers that
write shell scripts don't actually understand what they're doing, treating the
script as a necessary annoyance rather than a component of the software.

~~~
TheDong
If anyone understands shell scripts, it would be people writing init scripts
at Red Hat :)

Anyways, that is _not_ anything like other programming languages. Checking in
that way is error prone and not really an improvement (nor equivalent to set
-o).

    
    
      [[ "$DAEMON_PATH" ]] && rm -rf "$DEAMON_PATH/*"
    

See what I did there? It's an rm -rf /* bug because "checking variables" is
_not_ the answer.

In other programming languages, if an identifier is mis-typed things will blow
up. E.g., in ruby if I write:

    
    
      daemon_path=1; if daemon_path; puts deamon_path; end
    

I get "NameError: undefined local variable or method `deamon_path`"

These issues do not always stem from bad developers. Bash's defaults are _not_
safe in many ways and saying "people should just check the variable" isn't
helpful here.

~~~
bishc
Shameless plug for my language "bish" (compiles to bash) which aims to solve
many of these annoyances with shell scripting:
[https://github.com/tdenniston/bish](https://github.com/tdenniston/bish)

------
jedisct1
My good old trick to mitigate that is:

    
    
        touch /-@
    

I also always do it in my home directory:

    
    
        touch ~/-@
    

That's the first thing I do on a new host.

When accidentally running rm -f *, the command expands to -@ first, which is
not a valid option and makes the command fail before doing any harm

    
    
        rm: illegal option -- @
        usage: rm [-f | -i] [-dPRrvW] file ...

~~~
lambeosaurus
As someone who uses the commandline a lot but isn't exactly a wizard, why does
this work?

~~~
ulrikrasmussen
The * is expanded by the shell to a space-delimited list of filenames, but the
shell does not adequately escape filenames that can be misinterpreted as
arguments to 'rm'.

~~~
gmac
That's kind of scary — in that case I guess I should avoid creating a file
named -rf.

~~~
wodenokoto
That is really interesting.

Can someone knowledgeable about the shell expand on this? I don't dare test it
on my machine.

~~~
vbezhenar
Shell session to demonstrate (DO NOT DO IT IN THE DIRECTORY WITH IMPORTANT
FILES):

    
    
      $ touch important
      $ chmod 400 important
      $ rm *
      override r--------  vbezhenar/staff for important? n
      $ touch -- -rf
      $ ls -l
      total 0
      -rw-r--r--  1 vbezhenar  staff  0 Mar 24 14:35 -rf
      -r--------  1 vbezhenar  staff  0 Mar 24 14:35 important
      $ rm *
      $ ls -l
      total 0
      -rw-r--r--  1 vbezhenar  staff  0 Mar 24 14:35 -rf
      $ rm -- -rf

------
elchief
We had one of these kinda bugs.

If you uninstalled our software it deleted a major chunk of your Windows
registry, crippling your computer. It was a one character error in our script.
The first ticket read "Uninstalling [Product] destroys your computer".

I was responsible for customer support. Good times! Was a rough week. We
managed to not get sued.

~~~
VieElm
Pretty terrible that such a thing is even allowed by Windows. This is why as a
user I like Apple's OS X sandbox.

~~~
antientropic
Similar things have happened on OS X:
[http://apple.slashdot.org/story/01/11/04/0412209/itunes-20-i...](http://apple.slashdot.org/story/01/11/04/0412209/itunes-20-installer-
deletes-hard-drives)

~~~
coldtea
Parent said "This is why as a user I like Apple's OS X sandbox.".

This is a bug from 15 years ago, much much before the sandbox feature was
introduced.

A sandboxed iTunes would have prevented that.

~~~
makomk
A sandboxed iTunes would also prevent syncing your iPod and importing existing
music collections, because those both require access to files outside the
sandbox, which is probably why Apple hasn't done that.

~~~
aroch
Programs can read files outside their sandbox if they're asked to by user
input[1]. Sandboxing does not prevent interaction with USB devices

[1] See "Powerbox and File System Access Outside of Your Container" at
[https://developer.apple.com/library/mac/documentation/Securi...](https://developer.apple.com/library/mac/documentation/Security/Conceptual/AppSandboxDesignGuide/AppSandboxInDepth/AppSandboxInDepth.html)

------
ffn
Thanks OP, this actually made me laugh uproariously. Anyway, I'd be willing to
bet 100 push-ups that (unless it was malicious and not a bug), this thing is
caused by some clean up code somewhere that originally intended to do "rm -rf
/path/to/squid/socket" but the function that was suppose to generate the
"/path/to/squid/socket" string instead generated a null which was then
parseString'd onto a "" via some + function that was trying to do "/" \+ null.

But I'm neither a redhat user nor an OS dev, so I might completely wrong.

~~~
profmonocle
That's almost exactly how I blew up a test server once. (rsync --delete in
place of rm) Taught me to be extremely careful when dealing with absolute
directory paths.

~~~
marcosdumay
Oh man... Rsync deserves extreme caution even compared with other bash
commands. It's so easy to erase everything and copy terabytes again.

------
thaumaturgy
Hmm, there are two possible candidates in the init.d script for the RHEL 6
package of an older version of squid (doesn't look like bug submitter is using
a current version).

In stop():

    
    
        rm -rf $SQUID_PIDFILE_DIR/*
    

and in restart():

    
    
        rm -rf $SQUID_PIDFILE_DIR/*
    

SQUID_PIDFILE_DIR is hardcoded to "/var/run/squid" at the top of my copy of
the init script. But, neither of those rm commands check first to make sure
that SQUID_PIDFILE_DIR isn't empty (or, better yet, is in /var and doesn't
contain ".."), and either the submitter's copy of the script is mangled or
something else somewhere is stomping on SQUID_PIDFILE_DIR in the shell
environment.

...I should grep my init scripts for "rm".

~~~
rachelbythebay
squid-3.1.10-29.el6.src.rpm (from ftp.redhat.com, buried where they keep
SRPMs) has squid.init within, and that file has no mention of
SQUID_PIDFILE_DIR. A few other spot-checked versions are the same way.

[https://github.com/mozilla-services/squid-
rpm/blob/master/SO...](https://github.com/mozilla-services/squid-
rpm/blob/master/SOURCES/squid.init) ... however ... whatever that is, does.

Did they take this upstream init script somehow?

~~~
ato
This looks like it:

[https://bugzilla.redhat.com/show_bug.cgi?id=1102343](https://bugzilla.redhat.com/show_bug.cgi?id=1102343)

I guess they applied that change which was obviously written against a very
different init script where the variable is actually defined, got QA to test
it and immediately backed it out.

~~~
thaumaturgy
Oh, good call. Yeah, you found it.

------
zx2c4

        "Thanks Swapna and Red Hat QE for catching this issue before the package was released. Great work!"
    

Looks like this wasn't released into production.

~~~
stefanha
This comment should be at the top. False alarm, squid users, and now back to
our regularly scheduled HTTP proxying.

For more details on why this issue doesn't affect squid users:
[https://rwmj.wordpress.com/2015/03/24/restarting-squid-
resul...](https://rwmj.wordpress.com/2015/03/24/restarting-squid-results-in-
deleting-all-files-in-hard-drive/)

------
akandiah
It looks like the start-up/shutdown script is doing an rm -rf on a bash
variable that's evaluating to null.

~~~
smrtinsert
It's hard to trust anyone that would ever depend on such a variable without
having some sort of precondition.

~~~
TheDong
"it's hard to trust anyone that writes a dumb bug".

Listen to yourself. We've all written dumb bugs. We've all had that one line
of code that was an obvious mistake. I still trust people who write a bug here
and there because if I didn't I would have to forgo trusting everyone for
everyone makes dumb mistakes sometimes.

~~~
lmm
At this point I refuse to trust anyone who writes shell scripts. Bugs happen,
but a shell script is practically guaranteed to have zero automated tests and
variable-related bugs are all too common.

~~~
lttlrck
Surely your distrust should be aimed at shell scripts not at everyone who has
written one.

------
int19h
10 bucks says the lesson learned will be "remember kids, always set -u (and
other good ideas, like set -e, set -o pipefail, and personally I like set -o
posix but you'll need to give up process substitution for that)."

------
mplewis
I'm having trouble finding the related commit that fixed this, can anyone else
find it?

~~~
rwmj
This bug is an internal QA bug. The reporter is a Red Hat tester. The buggy
RPM was never released outside Red Hat, and so there's no requirement to
release the source. When RHEL 6.7 comes out the source will go up on
ftp.redhat.com.

------
anh79
Maybe I would add a new record to this [https://github.com/icy/bash-coding-
style#good-lessons](https://github.com/icy/bash-coding-style#good-lessons)

~~~
chanux
..aaand this bug happened in 2014 as well?

~~~
anh79
no, it's my mistake. I have fixed that.

------
fermigier
Scary. Even more scary is the fact that the bug has been open for a week, one
person has confirmed 100% reproducibility, and no one seems to care at Red
Hat. Isn't deleting peoples hard drives a big no no ?

~~~
cellularmitosis
Ahh Redhat, the distro which chose to symlink a bunch of binary lib files from
Apache into the etc directory.

"Why is grepping /etc taking so long? Binary files in /etc?!? WTF?!?"

Coming from Debian, Redhat seems to make a lot of irk-worthy choices.

~~~
lmm
As opposed to Debian, the distro which chooses to break tomcat (a program
which unzips into a single folder and is thereby completely self-contained) up
into a million different pieces and scatter them randomly all over your hard
drive?

~~~
ownagefool
To be fair to Debian, that's how it's supposed to work. You untar into /opt,
self contained, while your apt install puts things in the Filesystem Hierarchy
Standard, which means config goes into /etc.

You'll find most distros follow some FHS standard, although there's some
differences in interpretation.

