From dd4dcba832d420aec6c979b6e6eab2cda93f2b76 Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Tue, 20 Jan 2015 14:18:04 +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 Signed-off-by: Matej Habrnal Conflicts: tests/Makefile.am tests/testsuite.at --- src/include/dump_dir.h | 6 ++++++ src/lib/dump_dir.c | 25 +++++++++++++++++++------ tests/Makefile.am | 3 ++- tests/dump_dir.at | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/testsuite.at | 1 + 5 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 tests/dump_dir.at diff --git a/src/include/dump_dir.h b/src/include/dump_dir.h index 124511e..ed4c873 100644 --- a/src/include/dump_dir.h +++ b/src/include/dump_dir.h @@ -55,6 +55,12 @@ struct dump_dir { mode_t mode; time_t dd_time; char *dd_type; + + /* 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 045a45b..15e7dcd 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -216,6 +216,7 @@ int create_symlink_lockfile(const char* lock_file, const char* pid) 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)) @@ -243,6 +244,7 @@ int create_symlink_lockfile(const char* lock_file, const char* pid) } static const char *dd_check(struct dump_dir *dd) + { unsigned dirname_len = strlen(dd->dd_dirname); char filename_buf[FILENAME_MAX+1]; @@ -287,12 +289,18 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) int r = create_symlink_lockfile(lock_buf, pid_buf); if (r < 0) return r; /* error */ - if (r > 0) + if (r > 0 || errno == EALREADY) break; /* locked successfully */ /* Other process has the lock, wait for it to go away */ usleep(sleep_usec); } + /* 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 */ { @@ -304,8 +312,10 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) */ if (missing_file) { - xunlink(lock_buf); - log_notice("Unlocked '%s' (no or corrupted '%s' file)", lock_buf, missing_file); + if (dd->owns_lock) + xunlink(lock_buf); + + log_warning("Unlocked '%s' (no or corrupted '%s' file)", lock_buf, missing_file); if (--count == 0 || flags & DD_DONT_WAIT_FOR_LOCK) { errno = EISDIR; /* "this is an ordinary dir, not dump dir" */ @@ -324,13 +334,16 @@ static void dd_unlock(struct dump_dir *dd) { if (dd->locked) { - dd->locked = 0; - unsigned dirname_len = strlen(dd->dd_dirname); char lock_buf[dirname_len + sizeof("/.lock")]; strcpy(lock_buf, dd->dd_dirname); strcpy(lock_buf + dirname_len, "/.lock"); - xunlink(lock_buf); + + if (dd->owns_lock) + xunlink(lock_buf); + + dd->owns_lock = 0; + dd->locked = 0; log_info("Unlocked '%s'", lock_buf); } diff --git a/tests/Makefile.am b/tests/Makefile.am index 1cfc206..503c440 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -42,7 +42,8 @@ TESTSUITE_AT = \ report_python.at \ xfuncs.at \ string_list.at \ - ureport.at + ureport.at \ + dump_dir.at EXTRA_DIST += $(TESTSUITE_AT) TESTSUITE = $(srcdir)/testsuite diff --git a/tests/dump_dir.at b/tests/dump_dir.at new file mode 100644 index 0000000..efec63f --- /dev/null +++ b/tests/dump_dir.at @@ -0,0 +1,51 @@ +# -*- Autotest -*- + +AT_BANNER([dump_dir]) + +## -------------- ## +## 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; +} +]]) diff --git a/tests/testsuite.at b/tests/testsuite.at index abad32b..41107e7 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -17,3 +17,4 @@ m4_include([xml_definition.at]) m4_include([report_python.at]) m4_include([string_list.at]) m4_include([ureport.at]) +m4_include([dump_dir.at]) -- 2.1.0