From dd4dcba832d420aec6c979b6e6eab2cda93f2b76 Mon Sep 17 00:00:00 2001
From: Jakub Filak <jfilak@redhat.com>
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 <jfilak@redhat.com>
Signed-off-by: Matej Habrnal <mhabrnal@redhat.com>
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 <errno.h>
+#include <assert.h>
+
+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