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

[Allan McRae]

On 16/11/11 14:16, 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 and not over the original database
> directly. 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, zip the file to the same directory, prefixing the filename with
> '.tmp.'. We then move the file into place. This move is guaranteed to be
> atomic, so any reader of the database file will get either the old
> version, the new version, or ENOENT.
>
> We also perform a hardlink if possible instead of a move when shifting
> the old database out of the way to '.old'; 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 the 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>

Minor correction below.  Otherwise,

Signed-off-by: Allan


> ---
>   scripts/repo-add.sh.in |   38 ++++++++++++++++++++++++++------------
>   1 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in
> index 280c024..42c7334 100644
> --- a/scripts/repo-add.sh.in
> +++ b/scripts/repo-add.sh.in
> @@ -640,37 +640,51 @@ if (( success )); then
>   	msg "$(gettext "Creating updated database file '%s'")" "$REPO_DB_FILE"
>
>   	TAR_OPT=$(verify_repo_extension "$REPO_DB_FILE")
> +	# $LOCKFILE is already guaranteed to be absolute so this is safe
> +	dirname=${LOCKFILE%/*}
>   	filename=${REPO_DB_FILE##*/}
> +	# this ensures we create it on the same filesystem, making moves atomic
> +	tempname="$dirname/.tmp.$filename"
>
>   	pushd "$tmpdir/tree">/dev/null
>   	if ( shopt -s nullglob; files=(*); (( ${#files[*]} )) ); then
> -		bsdtar -c${TAR_OPT}f "$tmpdir/$filename" *
> +		bsdtar -c${TAR_OPT}f "$tempname" *
>   	else
>   		# we have no packages remaining? zip up some emptyness
>   		warning "$(gettext "No packages remain, creating empty database.")"
> -		bsdtar -c${TAR_OPT}f "$tmpdir/$filename" -T /dev/null
> +		bsdtar -c${TAR_OPT}f "$tempname" -T /dev/null
>   	fi
>   	popd>/dev/null
>
> -	create_signature "$tmpdir/$filename"
> +	create_signature "$tempname"
>
> -	[[ -f $REPO_DB_FILE ]]&&  mv -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old"
> +	# hardlink or move the previous version of the database and signature to .old
> +	# extension as a backup measure
> +	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"
> +
> +	# rotate the newly-created database and signature into place
> +	mv "$tempname" "$REPO_DB_FILE"
> +	if [[ -f .sig ]]; then

[[ -f $tempname.sig ]]


> +		mv "$tempname.sig" "$REPO_DB_FILE.sig"
> +	fi
> +
>   	dblink="${REPO_DB_FILE%.tar*}"
> -	target=${REPO_DB_FILE##*/}
>   	rm -f "$dblink" "$dblink.sig"
> -	ln -s "$target" "$dblink" 2>/dev/null || \
> -		ln "$target" "$dblink" 2>/dev/null || \
> +	ln -s "$filename" "$dblink" 2>/dev/null || \
> +		ln "$filename" "$dblink" 2>/dev/null || \
>   		cp "$REPO_DB_FILE" "$dblink"
>   	if [[ -f "$REPO_DB_FILE.sig" ]]; then
> -		ln -s "$target.sig" "$dblink.sig" 2>/dev/null || \
> -			ln "$target.sig" "$dblink.sig" 2>/dev/null || \
> +		ln -s "$filename.sig" "$dblink.sig" 2>/dev/null || \
> +			ln "$filename.sig" "$dblink.sig" 2>/dev/null || \
>   			cp "$REPO_DB_FILE.sig" "$dblink.sig"
>   	fi
>   else