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

[David Sterba <dsterba@suse.cz>]

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

I see from your and Dave's response that the problems are known and that
everybody tried to fix it in some way. What I also see that it's kind of
futile so that people carry their patches in own branches for testing.
The amount of treewide changes is probably killing the idea or this
would have to be coordinated or done in sprints or idk.

> > For example:
> > 
> > _rm_tmp() {
> >     rm -rf -- $tmp
> 
> Um, isn't this /also/ an unquoted variable?

That was not the best example, I took the existing code and put a
wrapper but yeah quoting should have been there.

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

Maybe if there are known patterns and helpers that avoid the unsafe
constructs this could be avoided. Then go trough all code, fix it one by
one and catch it in newly submitted code. It's too much handwaving now.
I think we'd know what to do but the questions are when and how to get
the changes merged.