On Wed, Feb 21, 2024 at 08:13:58AM -0800, Darrick J. Wong wrote: > 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. I think the "set -e" isn't suitable for fstests, it might bring in more troubles. About the "set -u", it's a good idea, but as I said in another reply, I think we need to evaluate its effection on fstests running, especially if we want to do it at the beginning of each case. There're too many cases in fstests, I don't know if some cases keep running with empty variables. What do you think? > > > 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! :)) The cleanup things need more improvement/enhancement, we can't perfect it in one patchset, so feel free to improve it bit by bit. > > --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. > > >
