Update: Here is my current corrected version (do not trust blindly, I had typos involved too!)

#!/usr/bin/env bash

TEMPDIR="$(mktemp -d)"
mkdir --verbose -- "${TEMPDIR}/tests"
trap 'cd -- "${TEMPDIR}/tests" && rm --verbose --one-file-system -rf "${TEMPDIR:-/invalid/615e1a5d}/tests"; cd ..; rmdir --verbose -- "${TEMPDIR}"' EXIT

And an alternative variant in case there are only files without subdirectories involved under "tests":

trap 'cd -- "${TEMPDIR}/tests" && rm --verbose --one-file-system -f -- "${TEMPDIR:-/invalid/615e1a5d}/tests/"*; cd ..; rmdir --verbose -- tests "${TEMPDIR}"' EXIT

Note, I use --verbose to explicitly list files, because this is for my Test system. If you copy this construct to use in your own normal scripts, you might want to remove the verbose flags for normal usage.


Down below is old version:

This is just a little small question if this is secure. This script is used to create a fresh test environment that should get deleted when script ends. trap command solves that issue fine. However, I am very, very afraid of doing rm -rf in context of variables, in case the variable happens to become empty due to user error (or later changes in script). So I will do this in multiple steps.

#!/usr/bin/env bash

TEMPDIR="$(mktemp -d)"
mkdir -f -- "${TEMPDIR}/tests"
trap 'cd -- "${TEMPDIR}/tests" && rm -rf tests && cd .. && rmdir -- ${TEMPDIR}' EXIT

# Here follows the script content, creating temporary files and manipulating them...
  1. Use a subdirectory, so the variable is not used by itself. So we have to use ${TEMPDIR}/tests each time instead just ${TEMPDIR}.
  2. When removing all files recursively, first enter into directory with cd, and only if that was successful delete all files recursively with a specific directory name. This should make sure that rm -rf is only executed if the temporary directory even exist and the variable is not resolved to empty.
  3. Off course go up one dir again and then remove the empty directory with rmdir, which will only remove empty directories.

I personally feel confident that this construct is safe, but would like to hear your opinions. Maybe I missed something important. It would be devastating. I don't want to try out various ways to see if one of them is working correctly.


Edit: For anyone who does not create uncontrolled temporary directories, they could just use rm -f tests/* instead, so nothing is deleted recursively. I may go that route and avoid sub-directories in my test folder.

you are viewing a single comment's thread
view the rest of the comments
[–] 3 points 1 week ago* (9 children)

I mean, multiple commands and changing directories vs. one command. Trust me here, that's more fragile.
And the advantage of rmdir over rm -r is, that it only deletes empty directories, is safer in some usecases. But this is moot, if you delete it's content anyway.

And if you do have multiple commands in a trap, i recommend exporting them to a function; easier to grok.

  • source
  • parent
  • hideshow 9 child comments
  • [–] [S] 0 points 1 week ago (8 children)

    I do not think that one command is more secure, compared to multiple steps to make sure it is secure. It can be, but it can be worse too.

    And the advantage of rmdir over rm -r is, that it only deletes empty directories, is safer in some usecases. But this is moot, if you delete it’s content anyway.

    Why is it safer in only some cases? I would always prefer deleting files without recursion and then deleting empty directories. Why is that moot? The point is not to use recursion.

  • source
  • parent
  • hideshow 8 child comments
  • [–] 0 points 1 week ago (7 children)

    You're already deleting files recursively:

    rm --verbose --one-file-system -rf "${TEMPDIR:-/invalid/615e1a5d}/tests"
    
  • source
  • parent
  • hideshow 7 child comments
  • [–] [S] 0 points 1 week ago (6 children)

    I don't use rm on a variable only, but with fixed path as $TEMPDIR/tests. Therefore the actual last empty $TEMPDIR needs to be deleted separately.

  • source
  • parent
  • hideshow 6 child comments
  • [–] 0 points 1 week ago* (5 children)

    But you're still deleting files with recursion, despite saying

    I would always prefer deleting files without recursion and then deleting empty directories. Why is that moot? The point is not to use recursion.

    So if you're using recursion to begin with, rm -rf "$TEMPDIR" is simpler than your rm -rf "$TEMPDIR/tests" && rmdir $TEMPDIR, and works identically except for when $TEMPDIR has other files. But it doesn't sound like that's the case.

    If you're always opposed to using recursion, why are you ok with using it to remove the subdirectory?

  • source
  • parent
  • hideshow 5 child comments
  • [–] [S] 1 point 1 week ago (4 children)

    That's the point, I do not want to do rm -rf "$TEMPDIR", which is a variable only. Adding a fixed string like "/tests" makes sure that recursive deletion never operates on a variable only. That has the sideffect that the $TEMPDIR itself isn't deleted, so I have to do it manually with rmdir.

  • source
  • parent
  • hideshow 4 child comments
  • [–] 1 point 1 week ago (3 children)

    I'm asking why, though. The "variable only" thing doesn't make sense. If you're worried about it being unset or not pointing to a directory, use test / [ with -n or -d. If you can't trust your own script to not change the variable, that's a hell of a threat model and you shouldn't remove anything.

  • source
  • parent
  • hideshow 3 child comments
  • [–] [S] 1 point 1 week ago (2 children)

    It's not just about being empty, but it could point to a different directory.

  • source
  • parent
  • hideshow 2 child comments
  • [–] 1 point 1 week ago (1 child)

    Then if you're wanting to only remove the directory if it has a tests subdir, the typical way would be to check [ -d "$TEMPDIR"/tests ] before recursively removing $TEMPDIR. No need for the cds or double remove.

  • source
  • parent
  • hideshow 1 child comment
  • [–] [S] 1 point 1 week ago

    If $TEMPDIR was changed by accident to "." then [ -d "$TEMPDIR"/tests ] could in example point to "./tests" (or any other directory that has "tests"). Then the directory would exists and then I would continue to rm -rf . or anything it points to. I don't see how your way is more secure, than my way.

  • source
  • parent