Dangers of using rm command with variables

Question

I am creating a crontab that compresses 15 minute clips from my security camera into one file (24 Hours Long) and then having the clips delete.

avimerge -o /media/jmartin/Cams/video/Full_$(date +%F --date "Yesterday") -i /media/jmartin/Cams/video/$(date +%F --date "Yesterday")* # Converts files from the past 24 hours into one .avi

rm /media/jmartin/Cams/video/$(date +%F --date "Yesterday")* # Removes old clips that have already been compressed

My question is What is the danger of using the $date variable. Could something possibly happen where it deletes all files in /video/? What would you recommend as a safer alternative?

Example filenames (Yes, those are spaces in the filename):

2016-04-25 00:00:01.avi  
2016-04-25 00:15:02.avi 
2016-04-25 00:30:02.avi  
2016-04-25 00:45:01.avi

a word from someone who has lost some invaluable photos in a similar situation like this: Instead of deleting files, consider acquiring some free or very inexpensive storage space and move your files there. Since they are security camera feed videos, I am pretty sure they will be obsolete in 30-60 days time period. Once every 30-60 days, go to your storage space and delete everything manually. If for nothing, just for the peace of mind in my opinion. — MelBurslan, Apr 25 '16 at 15:17
if possible add a constatnt prefix to your clips filename, like clip-2016-04-25 00:00:01.avi. That way when deleting .../clip-$(date ...)* files you don't risk to erase the other files. — Emmanuel, Apr 25 '16 at 15:23

score 5 · Answer 1 · answered Apr 25 '16 at 15:14

5

The $(date +%F --date "Yesterday") technically isn't a variable, it's a command substitution, but that is tangential to your question. This construct could prove to be problematic if for some reason the date command wasn't in your $PATH, and thusreturned nothing - at which point it would delete everything in /video/. If you instead take that command substitution and assign it to a variable prior to the avimerge command, then use that variable in both the avimerge and rm commands, you're not only ensuring that the date string being operated on doesn't change, but you're also able to test for a zero-length variable before either command and (if you have a zero-length string) exit before doing something you don't want to.

answered Apr 25 '16 at 15:14

John

17,011

So are you suggesting i set DATE = /bin/date +%F --date "Yesterday" and then run rm /media/jmartin/Cams/video/$DATE* – SimplePi Apr 25 '16 at 15:20
+1 for pointing out that a single date value should be generated and used for all related commands, instead of several potentially conflicting ones. – Guido Apr 25 '16 at 15:21
@SimplePi: yes, that is what I'm suggesting, in addition to sanity checking the value of $DATE after you establish it. – John Apr 25 '16 at 16:55

score 2 · Accepted Answer · answered Apr 25 '16 at 15:23

2

Two things jump out:

You have no checking for failure of the substitution
There is a race condition if the date changes between uses of the date command.

You could solve them both like this:

#/bin/bash

# Exit if any command fails
set -e

dir='/media/jmartin/Cams/video'
day=$(date +%F --date Yesterday)

# Conbine files from the past 24 hours into a single AVI file
avimerge -o "$dir/Full_$day" -i "$dir/$day"*

# Remove old clips that have already been compressed
rm "$dir/$day"*

answered Apr 25 '16 at 15:23

Toby Speight

8,678

if for some reason the $dir and $day are empty, The final command would result in an rm /*. on a short script it is not a problem but on longer, possibly managed by other people that can be dangerous – Emmanuel Apr 25 '16 at 16:00
@Emmanuel How do you suggest I go about fixing that. That would be an absolute nightmare. – SimplePi Apr 25 '16 at 21:49
@SimplePi you could test if the vars are empty or add something constant in the path like rm "$dir/clip-$day"* to protect the path. Another possibility would be to test in a loop each file to see if the name matches your format. – Emmanuel Apr 26 '16 at 10:42
Just for clarification, in my answer, the variables will never be empty - dir is set to a fixed, non-empty value, and day will have a value unless date errors (which will abort the script due to set -e). – Toby Speight Apr 26 '16 at 10:45
1

@TobySpeight You can write a script that is perfect at the origin, then modify it later to add features, and do it again and again. If it don't includes some security at the rm level, you have a risk. On your script the risk is minimal because it is small but I remember a small script that deleted /bin/cat on 500 servers because of a forgotten backcote. – Emmanuel Apr 26 '16 at 11:01

score 1 · Answer 3 · edited Apr 25 '16 at 22:44

Since there is always a space in your filenames, I'd include that in your command:

rm "$(date +%F --date "Yesterday") "* # Removes old clips

That should be an easy way to prevent deleting all files in the directory, as even if date doesn't return anything, it would only delete files starting with a space character (which hopefully do not exist).

However there are several other dangers, for example

avimerge might fail in some way and you end up deleting files that haven't been merged...
avimerge might take considerable time and by the time rm runs, yesterday is today and you end up deleting the wrong stuff...

Basically it's a bad idea to blindly trust that these command did what you wanted them to. For automated deletion, you should check and double check everything and make sure to use your variables properly.

You should put the date result in an actual variable, check what the variable looks like, and then use that same variable for both commands so it can't change in between.

You should check the return codes (exit codes) of the commands you call and only proceed when there is no error returned by them (or specifically handle errors you are expecting).

You should check that the merged file was created and has a realistic file size.

score 1 · Answer 4 · edited Apr 13 '17 at 12:36

As long as you get the script right and you don't run it in a weird environment, everything should be fine.

But if something breaks the expected output of date, all bets are off. For example, if you run this snippet with IFS=-, then you'll be running something like

rm /media/… 04 26*

i.e. delete files beginning with 26 in the current directory. Of course this is just due to a rookie mistake in your script: you forgot the double quotes around the command substitution.

rm "/media/jmartin/Cams/video/$(date +%F --date "Yesterday")"*

This, at least, is guaranteed not to delete files outside of the target directory.

A safer approach here would be to do a staged cleanup. Then if something goes wrong you have some time to notice.

#!/bin/sh
set -e
cd /media/jmartin/Cams/video
if [ -d yesterday ]; then
  find yesterday -type f -delete
else
  mkdir yesterday
fi
mv "$(date +%F --date "Yesterday") "* yesterday/

Dangers of using rm command with variables

4 Answers4