From e7d2d704fe4befc028257e5f88a97b810a423449 Mon Sep 17 00:00:00 2001 From: David Steele Date: Fri, 29 May 2015 16:31:12 -0400 Subject: [PATCH] Fixed issue #91: Race condition in async archive-push. --- README.md | 2 ++ doc/doc.xml | 3 +++ lib/BackRest/Archive.pm | 10 +++++++--- lib/BackRest/Exception.pm | 3 ++- lib/BackRest/Lock.pm | 9 ++++++++- 5 files changed, 22 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 853b9ea3b..f050dac36 100644 --- a/README.md +++ b/README.md @@ -751,6 +751,8 @@ example: db-path=/data/db * Fixed an issue in async archiving where archive-push was not properly returning 0 when archive-max-mb was reached and moved the async check after transfer to avoid having to remove the stop file twice. Also added unit tests for this case and improved error messages to make it clearer to the user what went wrong. Reported by Michael Renner. +* Fixed a locking issue that could allow multiple operations of the same type against a single stanza. This appeared to be benign in terms of data integrity but caused spurious errors while archiving and could lead to errors in backup/restore. Reported by Michael Renner. + * Replaced JSON module with JSON::PP which ships with core Perl. ### v0.65: Improved resume and restore logging, compact restores diff --git a/doc/doc.xml b/doc/doc.xml index e4ec8edaa..777fd840a 100644 --- a/doc/doc.xml +++ b/doc/doc.xml @@ -709,6 +709,9 @@ Run a full backup on the db stanza. --type can Fixed an issue in async archiving where archive-push was not properly returning 0 when archive-max-mb was reached and moved the async check after transfer to avoid having to remove the stop file twice. Also added unit tests for this case and improved error messages to make it clearer to the user what went wrong. Reported by Michael Renner. + + Fixed a locking issue that could allow multiple operations of the same type against a single stanza. This appeared to be benign in terms of data integrity but caused spurious errors while archiving and could lead to errors in backup/restore. Reported by Michael Renner. + Replaced JSON module with JSON::PP which ships with core Perl. diff --git a/lib/BackRest/Archive.pm b/lib/BackRest/Archive.pm index f23a09848..d73bc0bca 100644 --- a/lib/BackRest/Archive.pm +++ b/lib/BackRest/Archive.pm @@ -385,15 +385,19 @@ sub pushProcess # Else the no-fork flag has been specified for testing else { - &log(INFO, 'No fork on archive local for TESTING'); + &log(DEBUG, 'No fork on archive local for TESTING'); } # Start the async archive push - &log(INFO, 'starting async archive-push'); + &log(DEBUG, 'starting async archive-push'); } # Create a lock file to make sure async archive-push does not run more than once - lockAcquire(operationGet()); + if (!lockAcquire(operationGet(), false)) + { + &log(DEBUG, 'another async archive-push process is already running - exiting'); + return 0; + } # Open the log file log_file_set(optionGet(OPTION_REPO_PATH) . '/log/' . optionGet(OPTION_STANZA) . '-archive-async'); diff --git a/lib/BackRest/Exception.pm b/lib/BackRest/Exception.pm index 1255c3e96..1aeef6240 100644 --- a/lib/BackRest/Exception.pm +++ b/lib/BackRest/Exception.pm @@ -38,6 +38,7 @@ use constant ERROR_PATH_CREATE => 122, ERROR_OPERATION_INVALID => 123, ERROR_HOST_CONNECT => 124, + ERROR_LOCK_ACQUIRE => 125, ERROR_UNKNOWN => 199 }; @@ -47,7 +48,7 @@ our @EXPORT = qw(ERROR_ASSERT ERROR_CHECKSUM ERROR_CONFIG ERROR_FILE_INVALID ERR ERROR_OPTION_DUPLICATE_KEY ERROR_OPTION_NEGATE ERROR_OPTION_REQUIRED ERROR_POSTMASTER_RUNNING ERROR_PROTOCOL ERROR_RESTORE_PATH_NOT_EMPTY ERROR_FILE_OPEN ERROR_FILE_READ ERROR_PARAM_REQUIRED ERROR_ARCHIVE_MISMATCH ERROR_ARCHIVE_DUPLICATE ERROR_VERSION_NOT_SUPPORTED ERROR_PATH_CREATE ERROR_OPERATION_INVALID ERROR_HOST_CONNECT - ERROR_UNKNOWN); + ERROR_UNKNOWN ERROR_LOCK_ACQUIRE); #################################################################################################################################### # CONSTRUCTOR diff --git a/lib/BackRest/Lock.pm b/lib/BackRest/Lock.pm index 3f37fefa4..f9e747c50 100644 --- a/lib/BackRest/Lock.pm +++ b/lib/BackRest/Lock.pm @@ -62,6 +62,7 @@ sub lockFileName sub lockAcquire { my $strLockType = shift; + my $bFailOnNoLock = shift; # Cannot proceed if a lock is currently held if (defined($strCurrentLockType)) @@ -80,12 +81,18 @@ sub lockAcquire $strCurrentLockFile = lockFileName($strLockType, optionGet(OPTION_STANZA), optionGet(OPTION_REPO_PATH)); sysopen($hCurrentLockHandle, $strCurrentLockFile, O_WRONLY | O_CREAT) - or confess &log(ERROR, "unable to open lock file ${strCurrentLockFile}"); + or confess &log(ERROR, "unable to open lock file ${strCurrentLockFile}", ERROR_FILE_OPEN); # Attempt to lock the lock file if (!flock($hCurrentLockHandle, LOCK_EX | LOCK_NB)) { close($hCurrentLockHandle); + + if (!defined($bFailOnNoLock) || $bFailOnNoLock) + { + confess &log(ERROR, "unable to acquire ${strLockType} lock", ERROR_LOCK_ACQUIRE); + } + return false; }