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...
- Use a subdirectory, so the variable is not used by itself. So we have to use
${TEMPDIR}/testseach time instead just${TEMPDIR}. - 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 thatrm -rfis only executed if the temporary directory even exist and the variable is not resolved to empty. - 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.
Some solutions:
or
or
or
My personal preference is the last one. Any time you call
rm -rfyou provide a default value to variables to ensure that if they are empty they get some other value instead.My reply is rejecting (most of) your suggestions, with reasons off course. I am glad you bring them up, so we can talk about it.
I would avoid
set -eoption, as I do not want he entire script to exit on error. So instead I can use theexitcommand when I really want to on specific errors. I rather would like to handle errors myself directly, maybe even not exiting, but displaying error code with$?in example.If anything, it would make more sense to just exit the script with
|| exit. In fact that is what I'm doing in the script after the trap command bycd "${TEMPDIR}/tests" || exit, so the script never continues without a successfulmktempdirectory.I always forget that Bash has default values for variables!
mktempactually makes sure it never returns an empty value. I'm not worried about what it returns, but that my script could change the value of$TEMPDIRby accident (in example to something empty). So assigning a default value aftermktempwill never have a chance to get the default value at all.This on the other hand I like a lot. Now I will not stop doing my other additional checks, but for good habit this can't be wrong. Maybe instead a custom directory name with an unlikely name, what about pointing it to
/dev/null? I actually like this idea and might incorporate it.I believe this is considered to be best practise, but I included the other options just for the sake of completeness.
Happy to help.
What if I would use /dev/null instead /invalid? Do you think this is a problem, better or worse?
rm -rf ${TEMPDIR:-/dev/null}. I will update the current solution above, but need some research first. Edit: Oh wait, that could be dangerous if. If the script runs with root privileges, then /dev/null would be deleted.As a regular user that's fine, but if your script might be run as root then there's a possibility that you delete the special file
/dev/nullwhich would cause a great deal of problems for your system and probably be hard to debug if you don't realise it has happened. I've heard of people doing this before so I think it is possible although I've never tried it.Edit: yeah I just saw your edit and I agree :)