[PATCH 1/8] abrt-handle-event: free more of allocated data

[Denys Vlasenko]

This patch doesn't change the logic of the program,
it only makes cleanup in is_crash_a_dup() and main()
more consistent, closing a few leaks:
* dump_dir_name is a result of realpath(), but wasn't freed.
* four instances of dd_close() are collapsed to one.
* dump_dir_name in main() also was leaking, fix it.
* printing of "DUP_OF_DIR: dir" is made simpler-looking.

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/daemon/abrt-handle-event.c | 61 +++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/src/daemon/abrt-handle-event.c b/src/daemon/abrt-handle-event.c
index e4da837..ea932b2 100644
--- a/src/daemon/abrt-handle-event.c
+++ b/src/daemon/abrt-handle-event.c
@@ -219,16 +219,15 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
     struct dump_dir *dd = dd_opendir(dump_dir_name, DD_OPEN_READONLY);
     if (!dd)
         return 0; /* wtf? (error, but will be handled elsewhere later) */
-
+    free(analyzer);
     analyzer = dd_load_text(dd, FILENAME_ANALYZER);
-    /* dump_dir_name can be relative */
-    dump_dir_name = realpath(dump_dir_name, NULL);
-
     dup_uuid_init(dd);
     dup_corebt_init(dd);
-
     dd_close(dd);
 
+    /* dump_dir_name can be relative */
+    dump_dir_name = realpath(dump_dir_name, NULL);
+
     DIR *dir = opendir(g_settings_dump_location);
     if (dir == NULL)
         goto end;
@@ -236,7 +235,7 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
     /* Scan crash dumps looking for a dup */
     //TODO: explain why this is safe wrt concurrent runs
     struct dirent *dent;
-    while ((dent = readdir(dir)) != NULL)
+    while ((dent = readdir(dir)) != NULL && crash_dump_dup_name == NULL)
     {
         if (dot_or_dotdot(dent->d_name))
             continue; /* skip "." and ".." */
@@ -244,6 +243,7 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
         if (ext && strcmp(ext, ".new") == 0)
             continue; /* skip anything named "<dirname>.new" */
 
+        dd = NULL;
         char *dump_dir_name2 = concat_path_file(g_settings_dump_location,
dent->d_name);
         char *dd_uid = NULL, *dd_analyzer = NULL;
 
@@ -258,7 +258,6 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
         dd_uid = dd_load_text_ext(dd, FILENAME_UID, DD_FAIL_QUIETLY_ENOENT);
         if (strcmp(uid, dd_uid))
         {
-            dd_close(dd);
             goto next;
         }
 
@@ -266,36 +265,35 @@ static int is_crash_a_dup(const char *dump_dir_name, void *param)
         dd_analyzer = dd_load_text_ext(dd, FILENAME_ANALYZER, DD_FAIL_QUIETLY_ENOENT);
         if (strcmp(analyzer, dd_analyzer))
         {
-            dd_close(dd);
             goto next;
         }
 
         if (dup_uuid_compare(dd)
          || dup_corebt_compare(dd)
         ) {
-            dd_close(dd);
             crash_dump_dup_name = dump_dir_name2;
+            dump_dir_name2 = NULL;
             retval = 1; /* "run_event, please stop iterating" */
-            goto end;
+            /* sonce crash_dump_dup_name != NULL now, we exit the loop */
         }
-        dd_close(dd);
 
 next:
         free(dump_dir_name2);
+        dd_close(dd);
         free(dd_uid);
         free(dd_analyzer);
     }
     closedir(dir);
 
 end:
-    free(analyzer);
+    free((char*)dump_dir_name);
     return retval;
 }
 
 static char *do_log(char *log_line, void *param)
 {
-    /* We pipe output of events to our log (which usually
-     * includes syslog). Otherwise, errors on post-create result in
+    /* We pipe output of events to our log.
+     * Otherwise, errors on post-create result in
      * "Corrupted or bad dump DIR, deleting" without adequate explanation why.
      */
     log("%s", log_line);
@@ -355,13 +353,21 @@ int main(int argc, char **argv)
         struct run_event_state *run_state = new_run_event_state();
         if (!interactive)
             make_run_event_state_forwarding(run_state);
+        run_state->logging_callback = do_log;
         if (post_create)
             run_state->post_run_callback = is_crash_a_dup;
-        run_state->logging_callback = do_log;
+
         int r = run_event_on_dir_name(run_state, dump_dir_name, event_name);
+
+        free_run_event_state(run_state);
+        /* Needed only if is_crash_a_dup() was called, but harmless
+         * even if it wasn't:
+         */
+        dup_uuid_fini();
+        dup_corebt_fini();
+
         if (r == 0 && run_state->children_count == 0)
             error_msg_and_die("No actions are found for event '%s'",
event_name);
-        free_run_event_state(run_state);
 
 //TODO: consider this case:
 // new dump is created, post-create detects that it is a dup,
@@ -372,24 +378,17 @@ int main(int argc, char **argv)
         /* Is crash a dup? (In this case, is_crash_a_dup() should have
          * aborted "post-create" event processing as soon as it saw uuid
          * and determined that there is another crash with same uuid.
-         * In this case it sets state.crash_dump_dup_name)
+         * In this case it sets crash_dump_dup_name)
          */
-        if (!crash_dump_dup_name)
-        {
-            /* No. Was there error on one of processing steps in run_event? */
-            if (r != 0)
-                return r; /* yes */
-        }
-        else
-        {
+        if (crash_dump_dup_name)
             error_msg_and_die("DUP_OF_DIR: %s", crash_dump_dup_name);
-        }
 
-        if (post_create)
-        {
-            dup_uuid_fini();
-            dup_corebt_fini();
-        }
+        /* Was there error on one of processing steps in run_event? */
+        if (r != 0)
+            return r; /* yes */
+
+        free(dump_dir_name);
+        dump_dir_name = NULL;
     }
 
     /* exit 0 means, that there is no duplicate of dump-dir */
-- 
1.8.1.4

[PATCH 2/8] abrt-server: add support for "POST /creation_notification"

[Denys Vlasenko]

The body of this request contains just the dirname of the new
problem dir.
Receipt of this command makes abrt-server run "post-create"
and then "notify[-dup]" on this dir.
The code for running "post-create" is taked from abtrd,
it is to be removed there later.

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/daemon/Makefile.am   |   3 +-
 src/daemon/abrt-server.c | 299 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 277 insertions(+), 25 deletions(-)

diff --git a/src/daemon/Makefile.am b/src/daemon/Makefile.am
index 74ff07f..bb8a5f6 100644
--- a/src/daemon/Makefile.am
+++ b/src/daemon/Makefile.am
@@ -40,9 +40,10 @@ abrt_server_SOURCES = \
 abrt_server_CPPFLAGS = \
     -I$(srcdir)/../include \
     -I$(srcdir)/../lib \
+    -DDEFAULT_DUMP_DIR_MODE=$(DEFAULT_DUMP_DIR_MODE) \
+    -DLIBEXEC_DIR=\"$(libexecdir)\" \
     $(GLIB_CFLAGS) \
     $(LIBREPORT_CFLAGS) \
-    -DDEFAULT_DUMP_DIR_MODE=$(DEFAULT_DUMP_DIR_MODE) \
     -D_GNU_SOURCE
 abrt_server_LDADD = \
     ../lib/libabrt.la \
diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c
index 27bb6d6..23552b3 100644
--- a/src/daemon/abrt-server.c
+++ b/src/daemon/abrt-server.c
@@ -168,22 +168,30 @@ static int create_problem_dir(GHashTable *problem_info, unsigned
pid)
     return 201; /* Created */
 }
 
-/* Remove dump dir */
-static int delete_path(const char *dump_dir_name)
+static bool dir_is_in_dump_location(const char *dump_dir_name)
 {
     unsigned len = strlen(g_settings_dump_location);
 
-    /* If doesn't start with "g_settings_dump_location/"... */
-    if (strncmp(dump_dir_name, g_settings_dump_location, len) != 0
-     || dump_dir_name[len] != '/'
-    /* or contains "/." anywhere (-> might contain ".." component)
*/
-     || strstr(dump_dir_name + len, "/.")
+    if (strncmp(dump_dir_name, g_settings_dump_location, len) == 0
+     && dump_dir_name[len] == '/'
+    /* must not contain "/." anywhere (IOW: disallow ".." component)
*/
+     && !strstr(dump_dir_name + len, "/.")
     ) {
+        return 1;
+    }
+    return 0;
+}
+
+/* Remove dump dir */
+static int delete_path(const char *dump_dir_name)
+{
+    /* If doesn't start with "g_settings_dump_location/"... */
+    if (!dir_is_in_dump_location(dump_dir_name))
+    {
         /* Then refuse to operate on it (someone is attacking us??) */
         error_msg("Bad problem directory name '%s', not deleting",
dump_dir_name);
         return 400; /* Bad Request */
     }
-
     if (!dump_dir_accessible_by_uid(dump_dir_name, client_uid))
     {
         if (errno == ENOTDIR)
@@ -191,7 +199,6 @@ static int delete_path(const char *dump_dir_name)
             error_msg("Path '%s' isn't problem directory",
dump_dir_name);
             return 404; /* Not Found */
         }
-
         error_msg("Problem directory '%s' can't be accessed by user with
uid %ld", dump_dir_name, (long)client_uid);
         return 403; /* Forbidden */
     }
@@ -201,6 +208,227 @@ static int delete_path(const char *dump_dir_name)
     return 0; /* success */
 }
 
+static pid_t spawn_event_handler_child(const char *dump_dir_name, const char *event_name,
int *fdp)
+{
+    char *args[6];
+    args[0] = (char *) LIBEXEC_DIR"/abrt-handle-event";
+    args[1] = (char *) "-e";
+    args[2] = (char *) event_name;
+    args[3] = (char *) "--";
+    args[4] = (char *) dump_dir_name;
+    args[5] = NULL;
+
+    int pipeout[2];
+    int flags = EXECFLG_INPUT_NUL | EXECFLG_OUTPUT | EXECFLG_QUIET | EXECFLG_ERR2OUT;
+    VERB1 flags &= ~EXECFLG_QUIET;
+
+    pid_t child = fork_execv_on_steroids(flags, args, pipeout,
+                                         /*env_vec:*/ NULL, /*dir:*/ NULL,
+                                         /*uid(unused):*/ 0);
+    if (fdp)
+        *fdp = pipeout[0];
+    return child;
+}
+
+static int run_post_create(const char *dirname)
+{
+    /* If doesn't start with "g_settings_dump_location/"... */
+    if (!dir_is_in_dump_location(dirname))
+    {
+        /* Then refuse to operate on it (someone is attacking us??) */
+        error_msg("Bad problem directory name '%s'", dirname);
+        return 400; /* Bad Request */
+    }
+    if (!dump_dir_accessible_by_uid(dirname, client_uid))
+    {
+        if (errno == ENOTDIR)
+        {
+            error_msg("Path '%s' isn't problem directory",
dirname);
+            return 404; /* Not Found */
+        }
+        error_msg("Problem directory '%s' can't be accessed by user with
uid %ld", dirname, (long)client_uid);
+        return 403; /* Forbidden */
+    }
+
+    int child_stdout_fd;
+    int child_pid = spawn_event_handler_child(dirname, "post-create",
&child_stdout_fd);
+
+    char *dup_of_dir = NULL;
+    struct strbuf *cmd_output = strbuf_new();
+
+    bool child_is_post_create = 1; /* else it is a notify child */
+
+ read_child_output:
+    //log("Reading from event fd %d", child_stdout_fd);
+
+    /* Read streamed data and split lines */
+    for (;;)
+    {
+        char buf[250]; /* usually we get one line, no need to have big buf */
+        errno = 0;
+        int r = safe_read(child_stdout_fd, buf, sizeof(buf) - 1);
+        if (r <= 0)
+            break;
+        buf[r] = '\0';
+
+        /* split lines in the current buffer */
+        char *raw = buf;
+        char *newline;
+        while ((newline = strchr(raw, '\n')) != NULL)
+        {
+            *newline = '\0';
+            strbuf_append_str(cmd_output, raw);
+            char *msg = cmd_output->buf;
+
+            /* Hmm, DUP_OF_DIR: ends up in syslog. move log() into 'else'? */
+            log("%s", msg);
+
+            if (child_is_post_create
+             && prefixcmp(msg, "DUP_OF_DIR: ") == 0
+            ) {
+                free(dup_of_dir);
+                dup_of_dir = xstrdup(msg + strlen("DUP_OF_DIR: "));
+            }
+
+            strbuf_clear(cmd_output);
+            /* jump to next line */
+            raw = newline + 1;
+        }
+
+        /* beginning of next line. the line continues by next read */
+        strbuf_append_str(cmd_output, raw);
+    }
+
+    /* EOF/error */
+
+    /* Wait for child to actually exit, collect status */
+    int status = 0;
+    if (safe_waitpid(child_pid, &status, 0) <= 0)
+	/* should not happen */
+        perror_msg("waitpid(%d)", child_pid);
+
+    /* If it was a "notify[-dup]" event, then we're done */
+    if (!child_is_post_create)
+        goto ret;
+
+    /* exit 0 means "this is a good, non-dup dir" */
+    /* exit with 1 + "DUP_OF_DIR: dir" string => dup */
+    if (status != 0)
+    {
+        if (WIFSIGNALED(status))
+        {
+            log("'post-create' on '%s' killed by signal %d",
+                            dirname, WTERMSIG(status));
+            goto delete_bad_dir;
+        }
+        /* else: it is WIFEXITED(status) */
+        if (!dup_of_dir)
+        {
+            log("'post-create' on '%s' exited with %d",
+                            dirname, WEXITSTATUS(status));
+            goto delete_bad_dir;
+        }
+    }
+
+    const char *work_dir = (dup_of_dir ? dup_of_dir : dirname);
+
+    /* Load problem_data (from the *first dir* if this one is a dup) */
+    struct dump_dir *dd = dd_opendir(work_dir, /*flags:*/ 0);
+    if (!dd)
+        /* dd_opendir already emitted error msg */
+        goto delete_bad_dir;
+
+    /* Update count */
+    char *count_str = dd_load_text_ext(dd, FILENAME_COUNT, DD_FAIL_QUIETLY_ENOENT);
+    unsigned long count = strtoul(count_str, NULL, 10);
+
+    /* Don't increase crash count if we are working with newly uploaded
+     * directory (remote crash) which already has its crash count set.
+     */
+    if ((status != 0 && dup_of_dir) || count == 0)
+    {
+        count++;
+        char new_count_str[sizeof(long)*3 + 2];
+        sprintf(new_count_str, "%lu", count);
+        dd_save_text(dd, FILENAME_COUNT, new_count_str);
+
+        /* This condition can be simplified to either
+         * (status * != 0 && * dup_of_dir) or (count == 1). But the
+         * chosen form is much more reliable and safe. We must not call
+         * dd_opendir() to locked dd otherwise we go into a deadlock.
+         */
+        if (strcmp(dd->dd_dirname, dirname) != 0)
+        {
+            /* Update the last occurrence file by the time file of the new problem */
+            struct dump_dir *new_dd = dd_opendir(dirname, DD_OPEN_READONLY);
+            char *last_ocr = NULL;
+            if (new_dd)
+            {
+                /* TIME must exists in a valid dump directory but we don't want to
die
+                 * due to broken duplicated dump directory */
+                last_ocr = dd_load_text_ext(new_dd, FILENAME_TIME,
+                            DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE |
DD_FAIL_QUIETLY_ENOENT);
+                dd_close(new_dd);
+            }
+            else
+            {   /* dd_opendir() already produced a message with good information about
failure */
+                error_msg("Can't read the last occurrence file from the new dump
directory.");
+            }
+
+            if (!last_ocr)
+            {   /* the new dump directory may lie in the dump location for some time */
+                log("Using current time for the last occurrence file which may be
incorrect.");
+                time_t t = time(NULL);
+                last_ocr = xasprintf("%lu", (long)t);
+            }
+
+            dd_save_text(dd, FILENAME_LAST_OCCURRENCE, last_ocr);
+
+            free(last_ocr);
+        }
+    }
+
+    /* Reset mode/uig/gid to correct values for all files created by event run */
+    dd_sanitize_mode_and_owner(dd);
+
+    dd_close(dd);
+
+    if (!dup_of_dir)
+        log("New problem directory %s, processing", work_dir);
+    else
+    {
+        log("Deleting problem directory %s (dup of %s)",
+                strrchr(dirname, '/') + 1,
+                strrchr(dup_of_dir, '/') + 1);
+        delete_dump_dir(dirname);
+    }
+
+    /* Run "notify[-dup]" event */
+    int fd;
+    child_pid = spawn_event_handler_child(
+                work_dir,
+                (dup_of_dir ? "notify-dup" : "notify"),
+                &fd
+    );
+    //log("Started notify, fd %d -> %d", fd, child_stdout_fd);
+    xmove_fd(fd, child_stdout_fd);
+    child_is_post_create = 0;
+    strbuf_clear(cmd_output);
+    free(dup_of_dir);
+    dup_of_dir = NULL;
+    goto read_child_output;
+
+ delete_bad_dir:
+    log("Deleting problem directory '%s'", dirname);
+    delete_dump_dir(dirname);
+
+ ret:
+    strbuf_free(cmd_output);
+    free(dup_of_dir);
+    close(child_stdout_fd);
+    return 0;
+}
+
 /* Checks if a string contains only printable characters. */
 static gboolean printable_str(const char *str)
 {
@@ -402,11 +630,11 @@ static int perform_http_xact(void)
     /* First line must be
"op<space>[http://host]/path<space>HTTP/n.n".
      * <space> is exactly one space char.
      */
-    if (strncmp(messagebuf_data, "DELETE ", strlen("DELETE ")) == 0)
+    if (prefixcmp(messagebuf_data, "DELETE ") == 0)
     {
         messagebuf_data += strlen("DELETE ");
         char *space = strchr(messagebuf_data, ' ');
-        if (!space || strncmp(space+1, "HTTP/", strlen("HTTP/")) !=
0)
+        if (!space || prefixcmp(space+1, "HTTP/") != 0)
             return 400; /* Bad Request */
         *space = '\0';
         //decode_url(messagebuf_data); %20 => ' '
@@ -419,16 +647,29 @@ static int perform_http_xact(void)
      * "PUT /" implies creation or replace of resource named "/"!
      * Delete PUT in 2014.
      */
-    if (strncmp(messagebuf_data, "PUT ", strlen("PUT ")) != 0
-     && strncmp(messagebuf_data, "POST ", strlen("POST ")) !=
0
+    if (prefixcmp(messagebuf_data, "PUT ") != 0
+     && prefixcmp(messagebuf_data, "POST ") != 0
     ) {
         return 400; /* Bad Request */
     }
 
+    enum {
+        CREATION_NOTIFICATION,
+        CREATION_REQUEST,
+    };
+    int url_type;
+    char *url = skip_non_whitespace(messagebuf_data) + 1; /* skip "POST " */
+    if (prefixcmp(url, "/creation_notification ") == 0)
+        url_type = CREATION_NOTIFICATION;
+    else if (prefixcmp(url, "/ ") == 0)
+        url_type = CREATION_REQUEST;
+    else
+        return 400; /* Bad Request */
+
     /* Read body */
     if (!body_start)
     {
-        VERB1 log("EOF detected, exiting");
+        log("Premature EOF detected, exiting");
         return 400; /* Bad Request */
     }
 
@@ -439,18 +680,21 @@ static int perform_http_xact(void)
     /* Loop until EOF/error/timeout */
     while (1)
     {
-        while (1)
+        if (url_type == CREATION_REQUEST)
         {
-            unsigned len = strnlen(messagebuf_data, messagebuf_len);
-            if (len >= messagebuf_len)
-                break;
-            /* messagebuf has at least one NUL - process the line */
-            process_message(problem_info, messagebuf_data);
-            messagebuf_len -= (len + 1);
-            memmove(messagebuf_data, messagebuf_data + len + 1, messagebuf_len);
+            while (1)
+            {
+                unsigned len = strnlen(messagebuf_data, messagebuf_len);
+                if (len >= messagebuf_len)
+                    break;
+                /* messagebuf has at least one NUL - process the line */
+                process_message(problem_info, messagebuf_data);
+                messagebuf_len -= (len + 1);
+                memmove(messagebuf_data, messagebuf_data + len + 1, messagebuf_len);
+            }
         }
 
-        messagebuf_data = xrealloc(messagebuf_data, messagebuf_len + INPUT_BUFFER_SIZE);
+        messagebuf_data = xrealloc(messagebuf_data, messagebuf_len + INPUT_BUFFER_SIZE +
1);
         int rd = read(STDIN_FILENO, messagebuf_data + messagebuf_len,
INPUT_BUFFER_SIZE);
         if (rd < 0)
         {
@@ -468,9 +712,16 @@ static int perform_http_xact(void)
             error_msg_and_die("Message is too long, aborting");
     }
 
-    /* Save problem dir. Don't let alarm to interrupt here */
+    /* Body received, EOF was seen. Don't let alarm to interrupt after this. */
     alarm(0);
 
+    if (url_type == CREATION_NOTIFICATION)
+    {
+        messagebuf_data[messagebuf_len] = '\0';
+        return run_post_create(messagebuf_data);
+    }
+
+    /* Save problem dir */
     int ret = 0;
     unsigned pid = convert_pid(problem_info);
     die_if_data_is_missing(problem_info);
-- 
1.8.1.4

[PATCH 3/8] abrt-handle-event: create DUMP_LOC/post-create.lock when running "post-create"

[Denys Vlasenko]

If lock can't be created fatally, such as DUMP_LOC doesn't exist,
or filesystem is RO, we won't run "post-create" at all.
Else, if there is another "post-create" holding it,
we wait for up to 100 seconds.

This change depends on create_symlink_lockfile() being exposed
in public API in libreport.

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/daemon/abrt-handle-event.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/src/daemon/abrt-handle-event.c b/src/daemon/abrt-handle-event.c
index ea932b2..8c97d70 100644
--- a/src/daemon/abrt-handle-event.c
+++ b/src/daemon/abrt-handle-event.c
@@ -290,6 +290,51 @@ end:
     return retval;
 }
 
+static void create_lockfile(void)
+{
+    char pid_str[sizeof(long)*3 + 4];
+    sprintf(pid_str, "%lu", (long)getpid());
+    char *lock_filename = concat_path_file(g_settings_dump_location,
"post-create.lock");
+
+    /* Someone else's post-create may take a long-ish time to finish.
+     * For example, I had a failing email sending there, it took
+     * a minute to time out.
+     * That's why timeout is large (100 seconds):
+     */
+    int count = 100;
+    while (1)
+    {
+        /* Return values:
+         * -1: error (in this case, errno is 0 if error message is already logged)
+         *  0: failed to lock (someone else has it locked)
+         *  1: success
+         */
+        int r = create_symlink_lockfile(lock_filename, pid_str);
+	if (r > 0)
+            break;
+	if (r < 0)
+            error_msg_and_die("Can't create '%s'", lock_filename);
+	if (--count == 0)
+        {
+            /* Someone else's post-create process is alive but stuck.
+             * Don't wait forever.
+             */
+            error_msg("Stale lock '%s', removing it", lock_filename);
+            xunlink(lock_filename);
+            break;
+        }
+        sleep(1);
+    }
+    free(lock_filename);
+}
+
+static void delete_lockfile(void)
+{
+    char *lock_filename = concat_path_file(g_settings_dump_location,
"post-create.lock");
+    xunlink(lock_filename);
+    free(lock_filename);
+}
+
 static char *do_log(char *log_line, void *param)
 {
     /* We pipe output of events to our log.
@@ -355,10 +400,16 @@ int main(int argc, char **argv)
             make_run_event_state_forwarding(run_state);
         run_state->logging_callback = do_log;
         if (post_create)
+        {
             run_state->post_run_callback = is_crash_a_dup;
+            create_lockfile();
+        }
 
         int r = run_event_on_dir_name(run_state, dump_dir_name, event_name);
 
+        if (post_create)
+            delete_lockfile();
+
         free_run_event_state(run_state);
         /* Needed only if is_crash_a_dup() was called, but harmless
          * even if it wasn't:
-- 
1.8.1.4

[PATCH 4/8] abrt-server: move create_problem_dir() function down. No code changes

[Denys Vlasenko]

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/daemon/abrt-server.c | 184 +++++++++++++++++++++++------------------------
 1 file changed, 92 insertions(+), 92 deletions(-)

diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c
index 23552b3..d3c8480 100644
--- a/src/daemon/abrt-server.c
+++ b/src/daemon/abrt-server.c
@@ -76,98 +76,6 @@ static unsigned total_bytes_read = 0;
 static uid_t client_uid = (uid_t)-1L;
 
 
-/* Create a new problem directory from client session.
- * Caller must ensure that all fields in struct client
- * are properly filled.
- */
-static int create_problem_dir(GHashTable *problem_info, unsigned pid)
-{
-    /* Create temp directory with the problem data.
-     * This directory is renamed to final directory name after
-     * all files have been stored into it.
-     */
-
-    gchar *dir_basename = g_hash_table_lookup(problem_info, "basename");
-    if (!dir_basename)
-        dir_basename = g_hash_table_lookup(problem_info, FILENAME_TYPE);
-
-    char *path = xasprintf("%s/%s-%s-%u.new",
-                           g_settings_dump_location,
-                           dir_basename,
-                           iso_date_string(NULL),
-                           pid);
-
-    /* This item is useless, don't save it */
-    g_hash_table_remove(problem_info, "basename");
-
-    /* No need to check the path length, as all variables used are limited,
-     * and dd_create() fails if the path is too long.
-     */
-    struct dump_dir *dd = dd_create(path, client_uid, DEFAULT_DUMP_DIR_MODE);
-    if (!dd)
-    {
-        error_msg_and_die("Error creating problem directory '%s'",
path);
-    }
-
-    dd_create_basic_files(dd, client_uid, NULL);
-    dd_save_text(dd, FILENAME_ABRT_VERSION, VERSION);
-
-    gpointer gpkey = g_hash_table_lookup(problem_info, FILENAME_CMDLINE);
-    if (!gpkey)
-    {
-        /* Obtain and save the command line. */
-        char *cmdline = get_cmdline(pid);
-        if (cmdline)
-        {
-            dd_save_text(dd, FILENAME_CMDLINE, cmdline);
-            free(cmdline);
-        }
-    }
-
-    /* Store id of the user whose application crashed. */
-    char uid_str[sizeof(long) * 3 + 2];
-    sprintf(uid_str, "%lu", (long)client_uid);
-    dd_save_text(dd, FILENAME_UID, uid_str);
-
-    GHashTableIter iter;
-    gpointer gpvalue;
-    g_hash_table_iter_init(&iter, problem_info);
-    while (g_hash_table_iter_next(&iter, &gpkey, &gpvalue))
-    {
-        dd_save_text(dd, (gchar *) gpkey, (gchar *) gpvalue);
-    }
-
-    dd_close(dd);
-
-    /* Move the completely created problem directory
-     * to final directory.
-     */
-    char *newpath = xstrndup(path, strlen(path) - strlen(".new"));
-    if (rename(path, newpath) == 0)
-        strcpy(path, newpath);
-    free(newpath);
-
-    log("Saved problem directory of pid %u to '%s'", pid, path);
-
-    /* Trim old problem directories if necessary */
-    load_abrt_conf();
-    if (g_settings_nMaxCrashReportsSize > 0)
-    {
-        /* x1.25 and round up to 64m: go a bit up, so that usual in-daemon trimming
-         * kicks in first, and we don't "fight" with it:
-         */
-        unsigned maxsize = g_settings_nMaxCrashReportsSize +
g_settings_nMaxCrashReportsSize / 4;
-        maxsize |= 63;
-        check_free_space(maxsize, g_settings_dump_location);
-        trim_problem_dirs(g_settings_dump_location, maxsize * (double)(1024*1024),
path);
-    }
-    free_abrt_conf_data();
-
-    free(path);
-
-    return 201; /* Created */
-}
-
 static bool dir_is_in_dump_location(const char *dump_dir_name)
 {
     unsigned len = strlen(g_settings_dump_location);
@@ -429,6 +337,98 @@ static int run_post_create(const char *dirname)
     return 0;
 }
 
+/* Create a new problem directory from client session.
+ * Caller must ensure that all fields in struct client
+ * are properly filled.
+ */
+static int create_problem_dir(GHashTable *problem_info, unsigned pid)
+{
+    /* Create temp directory with the problem data.
+     * This directory is renamed to final directory name after
+     * all files have been stored into it.
+     */
+
+    gchar *dir_basename = g_hash_table_lookup(problem_info, "basename");
+    if (!dir_basename)
+        dir_basename = g_hash_table_lookup(problem_info, FILENAME_TYPE);
+
+    char *path = xasprintf("%s/%s-%s-%u.new",
+                           g_settings_dump_location,
+                           dir_basename,
+                           iso_date_string(NULL),
+                           pid);
+
+    /* This item is useless, don't save it */
+    g_hash_table_remove(problem_info, "basename");
+
+    /* No need to check the path length, as all variables used are limited,
+     * and dd_create() fails if the path is too long.
+     */
+    struct dump_dir *dd = dd_create(path, client_uid, DEFAULT_DUMP_DIR_MODE);
+    if (!dd)
+    {
+        error_msg_and_die("Error creating problem directory '%s'",
path);
+    }
+
+    dd_create_basic_files(dd, client_uid, NULL);
+    dd_save_text(dd, FILENAME_ABRT_VERSION, VERSION);
+
+    gpointer gpkey = g_hash_table_lookup(problem_info, FILENAME_CMDLINE);
+    if (!gpkey)
+    {
+        /* Obtain and save the command line. */
+        char *cmdline = get_cmdline(pid);
+        if (cmdline)
+        {
+            dd_save_text(dd, FILENAME_CMDLINE, cmdline);
+            free(cmdline);
+        }
+    }
+
+    /* Store id of the user whose application crashed. */
+    char uid_str[sizeof(long) * 3 + 2];
+    sprintf(uid_str, "%lu", (long)client_uid);
+    dd_save_text(dd, FILENAME_UID, uid_str);
+
+    GHashTableIter iter;
+    gpointer gpvalue;
+    g_hash_table_iter_init(&iter, problem_info);
+    while (g_hash_table_iter_next(&iter, &gpkey, &gpvalue))
+    {
+        dd_save_text(dd, (gchar *) gpkey, (gchar *) gpvalue);
+    }
+
+    dd_close(dd);
+
+    /* Move the completely created problem directory
+     * to final directory.
+     */
+    char *newpath = xstrndup(path, strlen(path) - strlen(".new"));
+    if (rename(path, newpath) == 0)
+        strcpy(path, newpath);
+    free(newpath);
+
+    log("Saved problem directory of pid %u to '%s'", pid, path);
+
+    /* Trim old problem directories if necessary */
+    load_abrt_conf();
+    if (g_settings_nMaxCrashReportsSize > 0)
+    {
+        /* x1.25 and round up to 64m: go a bit up, so that usual in-daemon trimming
+         * kicks in first, and we don't "fight" with it:
+         */
+        unsigned maxsize = g_settings_nMaxCrashReportsSize +
g_settings_nMaxCrashReportsSize / 4;
+        maxsize |= 63;
+        check_free_space(maxsize, g_settings_dump_location);
+        trim_problem_dirs(g_settings_dump_location, maxsize * (double)(1024*1024),
path);
+    }
+    free_abrt_conf_data();
+
+    free(path);
+
+    return 201; /* Created */
+}
+
 /* Checks if a string contains only printable characters. */
 static gboolean printable_str(const char *str)
 {
-- 
1.8.1.4

[PATCH 5/8] abrt-server: make create_problem_dir() run "post-create"

[Denys Vlasenko]

Since abrtd will no longer run "post-create" for us,
we need to do it ourselves.

While at it, move free space check to be before dir creation,
not after; and removed redundant load_abrt_conf() call.

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/daemon/abrt-server.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c
index d3c8480..1d23063 100644
--- a/src/daemon/abrt-server.c
+++ b/src/daemon/abrt-server.c
@@ -343,6 +343,12 @@ static int run_post_create(const char *dirname)
  */
 static int create_problem_dir(GHashTable *problem_info, unsigned pid)
 {
+    /* Exit if free space is less than 1/4 of MaxCrashReportsSize */
+    if (g_settings_nMaxCrashReportsSize > 0)
+    {
+        check_free_space(g_settings_nMaxCrashReportsSize, g_settings_dump_location);
+    }
+
     /* Create temp directory with the problem data.
      * This directory is renamed to final directory name after
      * all files have been stored into it.
@@ -400,6 +406,9 @@ static int create_problem_dir(GHashTable *problem_info, unsigned pid)
 
     dd_close(dd);
 
+    /* Not needing it anymore */
+    g_hash_table_destroy(problem_info);
+
     /* Move the completely created problem directory
      * to final directory.
      */
@@ -410,23 +419,24 @@ static int create_problem_dir(GHashTable *problem_info, unsigned
pid)
 
     log("Saved problem directory of pid %u to '%s'", pid, path);
 
+    /* We let the peer know that problem dir was created successfully
+     * _before_ we run potentially long-running post-create.
+     */
+    printf("HTTP/1.1 201 Created\r\n\r\n");
+    fflush(NULL);
+    close(STDOUT_FILENO);
+    xdup2(STDERR_FILENO, STDOUT_FILENO); /* paranoia: don't leave stdout fd closed
*/
+
     /* Trim old problem directories if necessary */
-    load_abrt_conf();
     if (g_settings_nMaxCrashReportsSize > 0)
     {
-        /* x1.25 and round up to 64m: go a bit up, so that usual in-daemon trimming
-         * kicks in first, and we don't "fight" with it:
-         */
-        unsigned maxsize = g_settings_nMaxCrashReportsSize +
g_settings_nMaxCrashReportsSize / 4;
-        maxsize |= 63;
-        check_free_space(maxsize, g_settings_dump_location);
-        trim_problem_dirs(g_settings_dump_location, maxsize * (double)(1024*1024),
path);
+        trim_problem_dirs(g_settings_dump_location, g_settings_nMaxCrashReportsSize *
(double)(1024*1024), path);
     }
-    free_abrt_conf_data();
 
-    free(path);
+    run_post_create(path);
 
-    return 201; /* Created */
+    /* free(path); */
+    exit(0);
 }
 
 /* Checks if a string contains only printable characters. */
@@ -743,7 +753,8 @@ static int perform_http_xact(void)
 //...the problem being that problem_info here is not a problem_data_t!
 #endif
 
-    ret = create_problem_dir(problem_info, pid);
+    create_problem_dir(problem_info, pid);
+    /* does not return */
 
  out:
     g_hash_table_destroy(problem_info);
-- 
1.8.1.4

[PATCH 6/8] Stop dying in check_free_space(); rename it to low_free_space()

[Denys Vlasenko]

This allows us to still create compat coredump in abrt-hook-ccpp.
We used to just die if DUMP_LOC is too full.

While at it, removed x1.25 in abrt-hook-ccpp, it is vestigial
(we no longer do _trimming of old problems_ there.

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/daemon/abrt-server.c   | 3 ++-
 src/hooks/abrt-hook-ccpp.c | 9 +++------
 src/include/libabrt.h      | 5 ++---
 src/lib/hooklib.c          | 9 ++++++---
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c
index 1d23063..78f4814 100644
--- a/src/daemon/abrt-server.c
+++ b/src/daemon/abrt-server.c
@@ -346,7 +346,8 @@ static int create_problem_dir(GHashTable *problem_info, unsigned pid)
     /* Exit if free space is less than 1/4 of MaxCrashReportsSize */
     if (g_settings_nMaxCrashReportsSize > 0)
     {
-        check_free_space(g_settings_nMaxCrashReportsSize, g_settings_dump_location);
+        if (low_free_space(g_settings_nMaxCrashReportsSize, g_settings_dump_location))
+            exit(1);
     }
 
     /* Create temp directory with the problem data.
diff --git a/src/hooks/abrt-hook-ccpp.c b/src/hooks/abrt-hook-ccpp.c
index 99ce5d5..01006b2 100644
--- a/src/hooks/abrt-hook-ccpp.c
+++ b/src/hooks/abrt-hook-ccpp.c
@@ -618,12 +618,9 @@ int main(int argc, char** argv)
 
     if (g_settings_nMaxCrashReportsSize > 0)
     {
-        /* x1.25 and round up to 64m: go a bit up, so that usual in-daemon trimming
-         * kicks in first, and we don't "fight" with it:
-         */
-        unsigned maxsize = g_settings_nMaxCrashReportsSize +
g_settings_nMaxCrashReportsSize / 4;
-        maxsize |= 63;
-        check_free_space(maxsize, g_settings_dump_location);
+        /* If free space is less than 1/4 of MaxCrashReportsSize... */
+        if (low_free_space(g_settings_nMaxCrashReportsSize, g_settings_dump_location))
+            goto create_user_core;
     }
 
     /* Check /var/tmp/abrt/last-ccpp marker, do not dump repeated crashes
diff --git a/src/include/libabrt.h b/src/include/libabrt.h
index f033e21..9d90c19 100644
--- a/src/include/libabrt.h
+++ b/src/include/libabrt.h
@@ -44,16 +44,15 @@ int vdprintf(int d, const char *format, va_list ap);
 #endif
 
 
-#define check_free_space abrt_check_free_space
-
+#define low_free_space abrt_low_free_space
 /**
   @brief Checks if there is enough free space to store the problem data
 
   @param setting_MaxCrashReportsSize Maximum data size
   @param dump_location Location to check for the available space
 */
+int low_free_space(unsigned setting_MaxCrashReportsSize, const char *dump_location);
 
-void check_free_space(unsigned setting_MaxCrashReportsSize, const char *dump_location);
 #define trim_problem_dirs abrt_trim_problem_dirs
 void trim_problem_dirs(const char *dirname, double cap_size, const char *exclude_path);
 #define run_unstrip_n abrt_run_unstrip_n
diff --git a/src/lib/hooklib.c b/src/lib/hooklib.c
index 38a0e43..45beaa9 100644
--- a/src/lib/hooklib.c
+++ b/src/lib/hooklib.c
@@ -18,12 +18,13 @@
 #include <sys/statvfs.h>
 #include "libabrt.h"
 
-void check_free_space(unsigned setting_MaxCrashReportsSize, const char *dump_location)
+int low_free_space(unsigned setting_MaxCrashReportsSize, const char *dump_location)
 {
     struct statvfs vfs;
     if (statvfs(dump_location, &vfs) != 0)
     {
-        perror_msg_and_die("statvfs('%s')", dump_location);
+        perror_msg("statvfs('%s')", dump_location);
+        return 0;
     }
 
     /* Check that at least MaxCrashReportsSize/4 MBs are free */
@@ -33,9 +34,11 @@ void check_free_space(unsigned setting_MaxCrashReportsSize, const char
*dump_loc
     unsigned long fs_free_mb_x4 = ((unsigned long long)vfs.f_bfree / (1024/4)) *
vfs.f_bsize / 1024;
     if (fs_free_mb_x4 < setting_MaxCrashReportsSize)
     {
-        error_msg_and_die("Aborting dump: only %luMiB is available on %s",
+        error_msg("Only %luMiB is available on %s",
                           fs_free_mb_x4 / 4, dump_location);
+        return 1;
     }
+    return 0;
 }
 
 /* rhbz#539551: "abrt going crazy when crashing process is respawned".
-- 
1.8.1.4

[PATCH 7/8] abrt-hook-ccpp: send "POST /creation_notification" after creating problem dir

[Denys Vlasenko]

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/hooks/abrt-hook-ccpp.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/src/hooks/abrt-hook-ccpp.c b/src/hooks/abrt-hook-ccpp.c
index 01006b2..9186f06 100644
--- a/src/hooks/abrt-hook-ccpp.c
+++ b/src/hooks/abrt-hook-ccpp.c
@@ -19,6 +19,7 @@
     51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 */
 #include <syslog.h>
+#include <sys/un.h>
 #include <sys/utsname.h>
 #include "libabrt.h"
 
@@ -456,6 +457,38 @@ static int create_or_die(const char *filename)
     perror_msg_and_die("Can't open '%s'", filename);
 }
 
+static void notify_new_path(const char *path)
+{
+    int fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd < 0)
+    {
+        perror_msg("socket(AF_UNIX)");
+        return;
+    }
+
+    struct sockaddr_un sunx;
+    memset(&sunx, 0, sizeof(sunx));
+    sunx.sun_family = AF_UNIX;
+    strcpy(sunx.sun_path, VAR_RUN"/abrt/abrt.socket");
+
+    if (connect(fd, (struct sockaddr *)&sunx, sizeof(sunx)))
+    {
+        perror_msg("connect('%s')", sunx.sun_path);
+        close(fd);
+        return;
+    }
+
+    full_write_str(fd, "POST /creation_notification HTTP/1.1\r\n\r\n");
+    full_write_str(fd, path);
+    /*
+     * This sends FIN packet. Without it, close() may result in RST instead.
+     * Not really needed on AF_UNIX, just a bit of TCP-induced paranoia
+     * aka "good practice".
+     */
+    shutdown(fd, SHUT_WR);
+    close(fd);
+}
+
 int main(int argc, char** argv)
 {
     /* Kernel starts us with all fd's closed.
@@ -839,6 +872,8 @@ int main(int argc, char** argv)
 
         log("Saved core dump of pid %lu (%s) to %s (%llu bytes)", (long)pid,
executable, path, (long long)core_size);
 
+        notify_new_path(path);
+
         /* rhbz#539551: "abrt going crazy when crashing process is respawned"
*/
         if (g_settings_nMaxCrashReportsSize > 0)
         {
-- 
1.8.1.4

[PATCH 8/8] abrtd: disable inotify watch on DUMP_LOC

[Denys Vlasenko]

Signed-off-by: Denys Vlasenko <dvlasenk(a)redhat.com&gt;
---
 src/daemon/abrtd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/daemon/abrtd.c b/src/daemon/abrtd.c
index 79434d3..0315a0a 100644
--- a/src/daemon/abrtd.c
+++ b/src/daemon/abrtd.c
@@ -1088,6 +1088,7 @@ int main(int argc, char** argv)
         perror_msg_and_die("inotify_init failed");
     close_on_exec_on(inotify_fd);
 
+#if 0
     /* Watching 'g_settings_dump_location' for new files and delete self
      * because hooks expects that the dump location exists if abrtd is running
      */
@@ -1096,6 +1097,7 @@ int main(int argc, char** argv)
         perror_msg("inotify_add_watch failed on '%s'",
g_settings_dump_location);
         goto init_error;
     }
+#endif
     /* ...and upload dir */
     if (g_settings_sWatchCrashdumpArchiveDir)
     {
-- 
1.8.1.4

[Martin Milata]

Pushed, including the libreport patch.

I noticed that when processing a crash, following line appers in the
syslog (possibly several times):

abrt-server[10123]: Can't access '/var/tmp/abrt/post-create.lock': No such
file or directory

This is due to duplicate detection in abrt-handle-event which iterates
over all items in /var/tmp/abrt and fails to open the lock because it is
a broken symlink. While the log message could be avoided by adding
DD_FAIL_QUIETLY_EACCESS to the dd_opendir (or explicitly skipping the
lock), I was wondering why we don't store /proc/PID in the symlink
instead of just PID? This would make the symlink non-broken when the
locking process is still running.

Martin

[Jiri Moskovcak]

On 07/29/2013 03:49 PM, Martin Milata wrote:
>  Pushed, including the libreport patch.
>
>  I noticed that when processing a crash, following line appers in the
>  syslog (possibly several times):
>
>  abrt-server[10123]: Can't access '/var/tmp/abrt/post-create.lock': No such
> file or directory
>
>  This is due to duplicate detection in abrt-handle-event which iterates
>  over all items in /var/tmp/abrt and fails to open the lock because it is
>  a broken symlink. While the log message could be avoided by adding
>  DD_FAIL_QUIETLY_EACCESS to the dd_opendir (or explicitly skipping the
>  lock), I was wondering why we don't store /proc/PID in the symlink
>  instead of just PID? This would make the symlink non-broken when the
>  locking process is still running.
>
>  Martin
>  
Guys,
neither of those patches references any ticket which lead to those 
changes which breaks the commit rules! Don't push such patches!

Thanks,
Jirka

[Denys Vlasenko]

On 07/29/2013 04:23 PM, Jiri Moskovcak wrote:
>  On 07/29/2013 03:49 PM, Martin Milata wrote:
> > Pushed, including the libreport patch.
> >
> > I noticed that when processing a crash, following line appers in the
> > syslog (possibly several times):
> >
> > abrt-server[10123]: Can't access '/var/tmp/abrt/post-create.lock': No
> such file or directory
> >
> > This is due to duplicate detection in abrt-handle-event which iterates
> > over all items in /var/tmp/abrt and fails to open the lock because it is
> > a broken symlink. While the log message could be avoided by adding
> > DD_FAIL_QUIETLY_EACCESS to the dd_opendir (or explicitly skipping the
> > lock), I was wondering why we don't store /proc/PID in the symlink
> > instead of just PID? This would make the symlink non-broken when the
> > locking process is still running.
> >
> > Martin
> >
>  
>  Guys,
>  neither of those patches references any ticket which lead
>  to those changes which breaks the commit rules! Don't push such patches! 
Sorry.

Thiese patches were for the issue #657 "divide abrtd into two parts"

[Denys Vlasenko]

On 07/29/2013 03:49 PM, Martin Milata wrote:
>  Pushed, including the libreport patch.
>  
>  I noticed that when processing a crash, following line appers in the
>  syslog (possibly several times):
>  
>  abrt-server[10123]: Can't access '/var/tmp/abrt/post-create.lock': No such
> file or directory
>  
>  This is due to duplicate detection in abrt-handle-event which iterates
>  over all items in /var/tmp/abrt and fails to open the lock because it is
>  a broken symlink. 
Yes, here:

    while ((dent = readdir(dir)) != NULL && crash_dump_dup_name == NULL)
    {
...
        char *dump_dir_name2 = concat_path_file(g_settings_dump_location,
dent->d_name);
...
        dd = dd_opendir(dump_dir_name2, /*flags:*/ DD_FAIL_QUIETLY_ENOENT |
DD_OPEN_READONLY);
        if (!dd)
            goto next;


>  While the log message could be avoided by adding
>  DD_FAIL_QUIETLY_EACCESS to the dd_opendir (or explicitly skipping the
>  lock), 
I am preparing a patch which will make DD_FAIL_QUIETLY_ENOENT
(which is already used there) to work as intended.

>  I was wondering why we don't store /proc/PID in the symlink
>  instead of just PID? 
Then symlink will be a directory as far as stat() is concerned.
Probably an even worse situation :)