Re: Dangerous commands (was:[ANNOUNCE] fstests: for-next branch updated to v2024.02.04)

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Feb 21, 2024 at 03:09:51PM +0100, David Sterba wrote:
> Hi,
> 
> reading [1] and how late it was found that effectively a "rm -rf /" can
> happen makes me worried about what I can expect from fstests after git
> pull. Many people contribute and the number for custom _cleanup()
> functions with unquoted 'rm' commands is just asking for more problems.
> 
> [1] https://lore.kernel.org/all/20240205060016.7fgiyafbnrvf5chj@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/
> 
> Unquoted arguments in shell scripts is IMO a big anti-pattern,

That wasn't even the problem there.  I temporarily forgot my shellfu and
did the equivalent of:

<never set FUBAR>

rm -rf "$FUBAR"*

The shell helpfully expands the never-set FUBAR to an empty string, then
globs the star to everything in the current directory.  rm has
safeguards to prevent you from "rm -rf /" but that offers no protection
from what happens above.

The argument was quoted "properly", but what I think you're really
asking for is "set -u":

#!/bin/bash -u

unset FUBAR
echo -- rm -f "$FUBAR"*

$ /tmp/a.sh
/tmp/a.sh: line 8: FUBAR: unbound variable

Unfortunately you can't just set that unconditionally for all of fstests
and call it a day because now you have to test every line of shellcode
to make sure nothing breaks on unset variables that would have been fine
otherwise.  That turns into a mess of:

test -n "$FUBAR" && rm -f "$FUBAR"*

Oh but then there's the other bad shell habit of setting variables to
the empty string -- sometimes you really want that empty string, other
times authors seem to have used that in place of unset.  We've gotten
away with that bit of bad hygiene for ages, likely due to -u being a
nondefault option.

> unfortunately present everywhere in xfstests since the beginning.
> Rewriting all scripts would be quite a lot of work, could you at least
> provide safe versions of the cleanup helpers?

I worried about this a long time ago, and tried running shellcheck on
the entire codebase.  Thousands of error messages about sloppy quoting
later I gave up.  Later I turned that into a patch in djwong-wtf that
runs it only on the files changed by the head commit.

It **didn't notice** the cleanup error!  So that wouldn't have saved me.

Long term we ought to rewrite fstests in any language that isn't as much
of a foot gun.  Or someone starts a project to set -e -u and deals with
the massive treewide change that's going to be.

> For example:
> 
> _rm_tmp() {
>     rm -rf -- $tmp

Um, isn't this /also/ an unquoted variable?

I guess one could make safe(r) wrappers so at least rm won't get mixed
up by dashes at the start of filenames:

_rm_files() {
	rm -f -- "$@"
}

_rm_dirtree() {
	rm -r -- "$@"
}

But then you do

moopath="foo bar"
_rm_files $moopath

and we've lost the battle yet again.

(On the plus side, that's four separate "f*king bash" invocations in one
email! :))

--D

> }
> 
> and used as
> 
> _cleanup() {
>     _rm_tmp
> }
> 
> or at least mandate the "--" separator and quoting arguments in new code
> and gradually fix the existing code.
> 
> I can send patches at least for btrfs and generic as this affects me but
> first I'd like to know that this will become standard coding style
> requirement in fstests.
>