From d4b89774005c569d7fd576d0d0efa6b3dc877a4f Mon Sep 17 00:00:00 2001
From: Jakub Filak <jfilak@redhat.com>
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 <jfilak@redhat.com>
---
 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 <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;
+}
+]])
-- 
2.21.0