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.

  • thingsiplay@lemmy.mlOP
    0·
    3 days ago

    mktemp --suffix=$$ does not really solve the issue I have. I don’t want to use ${TEMPDIR}, but have something hardcoded when using the variable, as in rm -rf "${TEMPDIR}" vs rm -rf "${TEMPDIR}/tests". Because I’m not worried about what mktemp gives me back, but about when the variable is used at later time. The content of the variable could be altered after its initial creation.

    In your trap assert that the content of var TEMPDIR ends with your same current PID or fail before you clean anything up.

    I quiet don’t understand this point here. The trap command will run in any case the script exits, be a crash or normal exit. If the variable or directory is invalid, then the cleanup will not be executed. But that is by design, because I do not want to cleanup something that is not working correctly. I rather leave it to be cleaned up automatically with next reboot.

    You could also assert TEMPDIR is prefixed with $TMP or /tmp for even more robustness.

    It has already a fixed suffix part with “/tests” in use. I’m not worried about the TEMPDIR content if mktemp created it correctly. I’m more worried about the variable being altered and invalid at later point in the script. I would rather leave mktemp create the directory where it thinks is the best place (mostly it is /tmp, but that is not guaranteed). So prefixing the variable content itself doesn’t really solve the trust issues I have here, as it is not the creation time that I’m worried about.

    • IanTwenty@piefed.socialEnglish
      0·
      3 days ago

      I think we misunderstand each other, let me try and be clearer myself. The line I suggest is:

      TEMPDIR=$(mktemp --suffix -$$)

      Which will look something like this when run:

      TEMPDIR=/tmp/tmp.9qRCIGOb2k-30978

      …if the pid is 30978 for example.

      In the trap we can then check if TEMPDIR has been overwritten or not with:

      trap ‘( [[ $TEMPDIR = /tmp/*-$$ ]] && rm -rf $TEMPDIR ) || echo “TEMPDIR var overwritten! Cleanup skipped.”’’ EXIT

      I am on my phone so forgive if any syntax is not quite right.

      • thingsiplay@lemmy.mlOP
        0·
        3 days ago

        Ah I absolutely misunderstood you. The PID trick is actually clever!

        But I would still not solely want to rely on a $TEMPDIR variable alone, without a fixed path like "${TEMPDIR}/tests". The reason is, I am not just concerned about the trap cleanup, but also the usage in the script. I want never use the variable in the script (after setting up trap) without a fixed path on it. In example the script will change filenames, eventually using glob patterns or do other stuff. If the script is faulty and changes the temporary variable, then it will at least do this under “tests” no matter what.

        The idea is neat though. I have to think about this, experiment and see if I end up using this.