On Tue, Nov 15, 2011 at 5:57 PM, Dan McGee wrote:

This addresses a short but sweet race condition currently existing in repo-add and repo-remove. We do the smart thing and zip the database to a location in a temporary directory. However, we then proceed to move this file directly from the temporary directory to our final location, which is more than likely a cross-filesystem move (/tmp on tmpfs) and thus non-atomic.

Instead, break the move into two parts- first, move the file to a name with a '.tmp.' prefix in the correct directory, then move it to the correct name. This second move is guaranteed to be atomic, so any reader of the database file will get either the old version, the new version, or a file that doesn't exist.

We also perform a hardlink if possible instead of a move when shifting the old database out of the way; this ensures there is no chance of a database file not existing during the whole process.

Only one small race condition should now be present- when the database has been fully moved into place and th signature has not, you may see a mismatch. There seems to be no good way to address this, and it existed before this patch.

A final note- if someone had locked-down permissions on the directory that the database files are in (e.g., could only write to foo.db.tar.gz, foo.db, foo.db.tar.gz.old, foo.db.old, and the lock file), this would break.

Signed-off-by: Dan McGee ---

As I was writing the commit message for this I also realized we could just `bsdtar cf` straight to the '.tmp.' prefix and save ourselves a copy- this probably makes more sense.

Thoughts on the general ideas of the patch, besides the above simplification?

 scripts/repo-add.sh.in |   20 ++++++++++++++++----  1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index fc008fc..d3e72fd 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -655,14 +655,26 @@ if (( success )); then

       create_signature "$tmpdir/$filename"

-       [[ -f $REPO_DB_FILE ]] && mv -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old" +       if [[ -f $REPO_DB_FILE ]]; then +               ln -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old" 2>/dev/null || \ +                       mv -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old" +       fi        if [[ -f $REPO_DB_FILE.sig ]]; then -               mv -f "$REPO_DB_FILE.sig" "$REPO_DB_FILE.old.sig" +               ln -f "$REPO_DB_FILE.sig" "$REPO_DB_FILE.old.sig" 2>/dev/null || \ +                       mv -f "$REPO_DB_FILE.sig" "$REPO_DB_FILE.old.sig"        else                rm -f "$REPO_DB_FILE.old.sig"        fi -       [[ -f $tmpdir/$filename ]] && mv "$tmpdir/$filename" "$REPO_DB_FILE" -       [[ -f $tmpdir/$filename.sig ]] && mv "$tmpdir/$filename.sig" "$REPO_DB_FILE.sig" + +       # These odd double-moves ensure the database and signature files are seen all +       # or nothing and not halfway through a filesystem to filesystem copy. +       mv "$tmpdir/$filename" ".tmp.$REPO_DB_FILE" Ugh. Folks, don't try this at home, I'm an idiot. If $REPO_DB_FILE is a absolute path, or even one with a directory, this eats it bad. Will fix.

+       mv ".tmp.$REPO_DB_FILE" "$REPO_DB_FILE" +       if [[ -f $tmpdir/$filename.sig ]]; then +               mv "$tmpdir/$filename.sig" ".tmp.$REPO_DB_FILE.sig" +               mv ".tmp.$REPO_DB_FILE.sig" "$REPO_DB_FILE.sig" +       fi +        dblink="${REPO_DB_FILE%.tar*}"        target=${REPO_DB_FILE##*/}        rm -f "$dblink" "$dblink.sig" -- 1.7.7.3