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

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

On Sun, Feb 25, 2024 at 09:45:27AM -0800, Eric Biggers wrote:
> On Sun, Feb 25, 2024 at 09:03:04AM -0800, Darrick J. Wong wrote:
> > On Sun, Feb 25, 2024 at 08:51:28AM -0800, Eric Biggers wrote:
> > > On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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,
> > > > > 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?
> > > > 
> > > > Hi David,
> > > > 
> > > > Thanks for taking care about it :)
> > > > 
> > > > > 
> > > > > For example:
> > > > > 
> > > > > _rm_tmp() {
> > > > >     rm -rf -- $tmp
> > > > 
> > > > It's "$tmp.*"
> > > > 
> > > > May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> > > > "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> > > > of situation does the "--" hope to fix?
> > > > 
> > > > The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> > > > in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> > > > 
> > > > I thought some ways to avoid that:
> > > > 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> > > > 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> > > > 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> > > > does "cd /".
> > > > 4) Think about bringing in someone "Static program analysis" tool about bash
> > > > script, but I don't know if there're someone good, feel free to give me
> > > > suggestions.
> > > 
> > > "--" prevents the following arguments from being interpreted as options if they
> > > begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
> > > empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> > > ${FOO:?}.  Alternatively, 'set -u' can be used.
> > 
> > I said that four days ago.  Did nobody receive that reply?
> > 
> > https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b
> > 
> 
> You didn't mention the :? option, and I thought that would be worth mentioning.
> 
> Of course ideally -u would be used everywhere, as you said.

Yes, but why not reply to my reply instead of replying to the original
patch as if I'd never said anything at all?

The background on that -- I've been noticing the last couple of years
that every now and then I'll reply to something; then a day or two go
by; and then someone else will say the exact same thing I said.

However, they don't simply reply to my email with "Yes, what Darrick
said".  Often the reply literally reiterates what I said.  That makes me
feel invisible, which isn't great.  Then I do some digging and usually
find out that actually no, it's that Microsoft or Google or vger smtp
servers are either (a) broken or (b) their AI have decided that I am a
spammer or (c) maybe I actually /am/ in everyone's killfiles due to the
sheer volume of patches that I send to the lists.

Regardless, every time I see that I start worrying that email is broken
yet again.

That said, I'm not complaining about your specific behavior, Eric; I'm
putting out there that I don't trust our review process at all anymore.
<sadface>

--D

> - Eric
>