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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > 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. >
