[pacman-dev] [PATCH] repo-add: ensure database and signature files are always viewed in whole

[Dan McGee]

On Tue, Nov 15, 2011 at 5:57 PM, Dan McGee <dan at archlinux.org> 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 <dan at archlinux.org>
> ---
>
> 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
>
>