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.

  • arthropod_shift@programming.dev
    21·
    2 days ago

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

    I get the feeling that either there’s some missing info, a misunderstanding on my part, or there might be a simpler way to do things. Is $TEMPDIR changing a reasonable case to handle? Can you change your script design to make this impossible instead? (Like replacing a source with a script execution?)

    If the snippet is just boilerplate at the top of numerous scripts, I’d do set -u, rm -rf "${TEMPDIR}" on exit (ideally defined in a common setup script/function), and just avoid assigning to the var later in the script. In terms of defensive programming, anything extra is added complexity that will only make an error more likely imo. You could rename TEMPDIR to something like TEST_ROOT if you’re concerned about the variable name being accidentally used again, but no amount of trap logic is going to make a future programming error impossible.

    • thingsiplay@lemmy.mlOP
      11·
      2 days ago

      I know, good habit, check variable and so on. But mistakes happen, so what’s wrong with hardening the case? There are decades of studies and good practice for coding, yet the best programmers still do mistakes. And having a script that could potentially delete files on your system (even all subdirectories) is dangerous and should be handled with respect. I really don’t understand the opposition here.

      In terms of defensive programming, anything extra is added complexity that will only make an error more likely imo.

      I disagree here. Adding checks will help in catching those errors. Being not defensive about it will make it only more likely to make errors.

      no amount of trap logic is going to make a future programming error impossible.

      No amount of any programming will make it impossible to error out. That does not mean we shouldn’t try to make it as secure as we can think. Renaming the variable is putting the risk to another name, not really solving the issue.

      • arthropod_shift@programming.dev
        11·
        2 days ago

        I think you mistook my asking questions as opposition, or maybe my intent wasn’t clear enough in my comment. My main point was that while I don’t have full context for the problem, it seems to me that a simpler solution exists. Is there some edge case that the cd -> rm -rf -> rmdir snippet covers that’s missed by set -u -> rm -rf?

        • thingsiplay@lemmy.mlOP
          11·
          2 days ago

          I generally dislike using the set methods to change how the “language” works. So even if it covers my issue, I’m not using it. Its also not even said that these options couldn’t be changed, in fact I think in some cases it can be useful to change those options temporarily for certain effects like “pipefail” or the one that prints the executed lines. But not as a default or for critical commands that can run at any time the script exits (even on error). Maybe someone (even me) copy pastes this line in example.

          I always forget which of these set options do what, and next time when I write or read another script it could have different set of options. So I ignore those set options to change how the language is interpreted in Bash. Also if cd -> rm -rf -> rmdir solves it, why would I need to rewrite and change it to set -u -> rm -rf?

          • arthropod_shift@programming.dev
            2·
            2 days ago

            I generally dislike using the set methods to change how the “language” works. So even if it covers my issue, I’m not using it.

            If you don’t feel like using it, that’s valid, just as long as you’re aware of the functionality. It seemed fitting here to me because of the potential to simplify the code from a complex chain of commands with hard-coded values to a single command with no hard-coding, while keeping the old behavior.

            Also if cd -> rm -rf -> rmdir solves it, why would I need to rewrite and change it to set -u -> rm -rf?

            You don’t. You asked for feedback, though, so I gave some.

            In general, I’m a fan of set -u because it helps to avoid some common scripting bugs, but if you’re aware of the option and how it could be used, and you choose not to use it, then I’m not going to insist you write it how I would write it.