From d4b89774005c569d7fd576d0d0efa6b3dc877a4f Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Mon, 18 Mar 2019 12:26:37 +0100 Subject: [PATCH] dump_dir: allow (semi)recursive locking This patch only tries to mitigate the consequences of a bug in code where someone tries to lock a dump directory while it is already locked by the same process. This usually happens when a callee accepts a path to directory and opens it on its own or when someone forgets to call dd_unlock() or in all the unpredictable circumstance we usually have to face in ABRT. It is not possible to implement the lock counter using only a symbolic link and file system functions, thus I've decided to put the responsibility of unlocking to the first dd_lock() caller and disallow the consecutive callers to unlock the dump directory. Related to abrt/abrt#898 Signed-off-by: Jakub Filak --- src/include/dump_dir.h | 6 ++++++ src/lib/dump_dir.c | 21 +++++++++++++----- tests/dump_dir.at | 49 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/src/include/dump_dir.h b/src/include/dump_dir.h index badef17..b617c6c 100644 --- a/src/include/dump_dir.h +++ b/src/include/dump_dir.h @@ -71,6 +71,12 @@ struct dump_dir { time_t dd_time; char *dd_type; int dd_fd; + + /* In case of recursive locking the first caller owns the lock and is + * responsible for unlocking. The consecutive dd_lock() callers acquire the + * lock but are not able to unlock the dump directory. + */ + int owns_lock; }; void dd_close(struct dump_dir *dd); diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c index eb0c176..5e32c08 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -277,6 +277,7 @@ int create_symlink_lockfile_at(int dir_fd, const char* lock_file, if (strcmp(pid_buf, pid) == 0) { log("Lock file '%s' is already locked by us", lock_file); + errno = EALREADY; return 0; } if (isdigit_str(pid_buf)) @@ -359,7 +360,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) int r = create_symlink_lockfile_at(dd->dd_fd, ".lock", pid_buf, log_all_warnings); if (r < 0) return r; /* error */ - if (r > 0) + if (r > 0 || EALREADY == errno) break; /* locked successfully */ if (flags & DD_DONT_WAIT_FOR_LOCK) { @@ -371,6 +372,12 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) log_all_warnings = false; } + /* Reset errno to 0 only if errno is EALREADY (used by + * create_symlink_lockfile() to signal that the dump directory is already + * locked by us) */ + if (!(dd->owns_lock = (errno != EALREADY))) + errno = 0; + /* Are we called by dd_opendir (as opposed to dd_create)? */ if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */ { @@ -382,8 +389,10 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) */ if (missing_file) { - xunlinkat(dd->dd_fd, ".lock", /*only files*/0); - log_notice("Unlocked '%s' (no or corrupted '%s' file)", dd->dd_dirname, missing_file); + if (dd->owns_lock) + xunlinkat(dd->dd_fd, ".lock", /*only files*/0); + + log_warning("Unlocked '%s/.lock' (no or corrupted '%s' file)", dd->dd_dirname, missing_file); if (--count == 0 || flags & DD_DONT_WAIT_FOR_LOCK) { errno = EISDIR; /* "this is an ordinary dir, not dump dir" */ @@ -402,9 +411,11 @@ static void dd_unlock(struct dump_dir *dd) { if (dd->locked) { - dd->locked = 0; + if (dd->owns_lock) + xunlinkat(dd->dd_fd, ".lock", /*only files*/0); - xunlinkat(dd->dd_fd, ".lock", /*only files*/0); + dd->owns_lock = 0; + dd->locked = 0; log_info("Unlocked '%s/.lock'", dd->dd_dirname); } diff --git a/tests/dump_dir.at b/tests/dump_dir.at index dc95e5b..98d564c 100644 --- a/tests/dump_dir.at +++ b/tests/dump_dir.at @@ -566,3 +566,52 @@ TS_MAIN } TS_RETURN_MAIN ]]) + + +## -------------- ## +## recursive_lock ## +## -------------- ## + +AT_TESTFUN([recursive_lock], +[[ +#include "internal_libreport.h" +#include +#include + +int main(int argc, char **argv) +{ + g_verbose = 3; + + char *path = tmpnam(NULL); + struct dump_dir *dd = dd_create(path, -1L, DEFAULT_DUMP_DIR_MODE); + + char *lock_path = concat_path_file(path, ".lock"); + struct stat buf; + + assert(dd); + + assert(lstat(lock_path, &buf) == 0 && S_ISLNK(buf.st_mode)); + + dd_create_basic_files(dd, -1L, "/"); + dd_save_text(dd, "type", "custom"); + + struct dump_dir *dd2 = dd_opendir(path, DD_OPEN_READONLY); + assert(dd2->owns_lock == 0); + + struct dump_dir *dd3 = dd_opendir(path, 0); + assert(dd3->owns_lock == 0); + + dd_close(dd2); + assert(lstat(lock_path, &buf) == 0 && S_ISLNK(buf.st_mode)); + + dd_close(dd3); + assert(lstat(lock_path, &buf) == 0 && S_ISLNK(buf.st_mode)); + + dd_close(dd); + + assert(stat(lock_path, &buf) != 0 && errno == ENOENT); + free(lock_path); + + return 0; +} +]]) -- 2.21.0