abrt-hook-ccpp: eliminate race between process exit and compat coredump creation

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

1 files changed, 239 insertions, 125 deletions

diff --git a/src/Hooks/abrt-hook-ccpp.cpp b/src/Hooks/abrt-hook-ccpp.cpp
index 61277980..b72e8be8 100644
--- a/src/Hooks/abrt-hook-ccpp.cpp
+++ b/src/Hooks/abrt-hook-ccpp.cpp
@@ -41,6 +41,97 @@ static char* malloc_readlink(const char *linkname)
     return NULL;
 }
 
+/* Custom version of copyfd_xyz,
+ * one which is able to write into two descriptors at once.
+ */
+#define CONFIG_FEATURE_COPYBUF_KB 4
+static off_t copyfd_sparse(int src_fd, int dst_fd1, int dst_fd2, off_t size2)
+{
+	off_t total = 0;
+	int last_was_seek = 0;
+#if CONFIG_FEATURE_COPYBUF_KB <= 4
+	char buffer[CONFIG_FEATURE_COPYBUF_KB * 1024];
+	enum { buffer_size = sizeof(buffer) };
+#else
+	char *buffer;
+	int buffer_size;
+
+	/* We want page-aligned buffer, just in case kernel is clever
+	 * and can do page-aligned io more efficiently */
+	buffer = mmap(NULL, CONFIG_FEATURE_COPYBUF_KB * 1024,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANON,
+			/* ignored: */ -1, 0);
+	buffer_size = CONFIG_FEATURE_COPYBUF_KB * 1024;
+	if (buffer == MAP_FAILED) {
+		buffer = alloca(4 * 1024);
+		buffer_size = 4 * 1024;
+	}
+#endif
+
+	while (1) {
+		ssize_t rd = safe_read(src_fd, buffer, buffer_size);
+		if (!rd) { /* eof */
+			if (last_was_seek) {
+				if (lseek(dst_fd1, -1, SEEK_CUR) < 0
+				 || safe_write(dst_fd1, "", 1) != 1
+				 || (dst_fd2 >= 0
+				     && (lseek(dst_fd2, -1, SEEK_CUR) < 0
+					 || safe_write(dst_fd2, "", 1) != 1
+				        )
+				    )
+				) {
+					perror_msg("write error");
+					total = -1;
+					goto out;
+				}
+			}
+			/* all done */
+			goto out;
+		}
+		if (rd < 0) {
+			perror_msg("read error");
+			total = -1;
+			goto out;
+		}
+
+		/* checking sparseness */
+		ssize_t cnt = rd;
+		while (--cnt >= 0) {
+			if (buffer[cnt] != 0) {
+				/* not sparse */
+				errno = 0;
+				ssize_t wr1 = full_write(dst_fd1, buffer, rd);
+				ssize_t wr2 = (dst_fd2 >= 0 ? full_write(dst_fd2, buffer, rd) : rd);
+				if (wr1 < rd || wr2 < rd) {
+					perror_msg("write error");
+					total = -1;
+					goto out;
+				}
+				last_was_seek = 0;
+				goto adv;
+			}
+		}
+		/* sparse */
+		xlseek(dst_fd1, rd, SEEK_CUR);
+		if (dst_fd2 >= 0)
+			xlseek(dst_fd2, rd, SEEK_CUR);
+		last_was_seek = 1;
+ adv:
+		total += rd;
+		size2 -= rd;
+		if (size2 < 0)
+			dst_fd2 = -1;
+	}
+ out:
+
+#if CONFIG_FEATURE_COPYBUF_KB > 4
+	if (buffer_size != 4 * 1024)
+		munmap(buffer, buffer_size);
+#endif
+	return total;
+}
+
 static char* get_executable(pid_t pid)
 {
     char buf[sizeof("/proc/%lu/exe") + sizeof(long)*3];
@@ -57,16 +148,100 @@ static char* get_cwd(pid_t pid)
     return malloc_readlink(buf);
 }
 
+static char core_basename[sizeof("core.%lu") + sizeof(long)*3] = "core";
+
+static int open_user_core(const char *user_pwd, uid_t uid, pid_t pid)
+{
+    struct passwd* pw = getpwuid(uid);
+    gid_t gid = pw ? pw->pw_gid : uid;
+    xsetegid(gid);
+    xseteuid(uid);
+
+    errno = 0;
+    if (user_pwd == NULL
+     || chdir(user_pwd) != 0
+    ) {
+        perror_msg("can't cd to %s", user_pwd);
+        return 0;
+    }
+
+    /* Mimic "core.PID" if requested */
+    char buf[] = "0\n";
+    int fd = open("/proc/sys/kernel/core_uses_pid", O_RDONLY);
+    if (fd >= 0)
+    {
+        read(fd, buf, sizeof(buf));
+        close(fd);
+    }
+    if (strcmp(buf, "1\n") == 0)
+    {
+        sprintf(core_basename, "core.%lu", (long)pid);
+    }
+
+    /* man core:
+     * There are various circumstances in which a core dump file
+     * is not produced:
+     *
+     * [skipped obvious ones]
+     * The process does not have permission to write the core file.
+     * ...if a file with the same name exists and is not writable
+     * or is not a regular file (e.g., it is a directory or a symbolic link).
+     *
+     * A file with the same name already exists, but there is more
+     * than one hard link to that file.
+     *
+     * The file system where the core dump file would be created is full;
+     * or has run out of inodes; or is mounted read-only;
+     * or the user has reached their quota for the file system.
+     *
+     * The RLIMIT_CORE or RLIMIT_FSIZE resource limits for the process
+     * are set to zero.
+     * [shouldn't it be checked by kernel? 2.6.30.9-96 doesn't, still
+     * calls us even if "ulimit -c 0"]
+     *
+     * The binary being executed by the process does not have
+     * read permission enabled. [how we can check it here?]
+     *
+     * The process is executing a set-user-ID (set-group-ID) program
+     * that is owned by a user (group) other than the real
+     * user (group) ID of the process. [TODO?]
+     * (However, see the description of the prctl(2) PR_SET_DUMPABLE operation,
+     * and the description of the /proc/sys/fs/suid_dumpable file in proc(5).)
+     */
+
+    /* Do not O_TRUNC: if later checks fail, we do not want to have file already modified here */
+    struct stat sb;
+    errno = 0;
+    int user_core_fd = open(core_basename, O_WRONLY | O_CREAT | O_NOFOLLOW, 0600); /* kernel makes 0600 too */
+    xsetegid(0);
+    xseteuid(0);
+    if (user_core_fd < 0
+     || fstat(user_core_fd, &sb) != 0
+     || !S_ISREG(sb.st_mode)
+     || sb.st_nlink != 1
+    /* kernel internal dumper checks this too: if (inode->i_uid != current->fsuid) <fail>, need to mimic? */
+    ) {
+        perror_msg("%s/%s is not a regular file with link count 1", user_pwd, core_basename);
+        return -1;
+    }
+    if (ftruncate(user_core_fd, 0) != 0) {
+        /* perror first, otherwise unlink may trash errno */
+        perror_msg("truncate %s/%s", user_pwd, core_basename);
+        return -1;
+    }
+
+    return user_core_fd;
+}
+
 int main(int argc, char** argv)
 {
-    int fd;
     struct stat sb;
 
     if (argc < 5)
     {
-        const char* program_name = argv[0];
-        error_msg_and_die("Usage: %s: DUMPDIR PID SIGNO UID CORE_SIZE_LIMIT", program_name);
+        error_msg_and_die("Usage: %s: DUMPDIR PID SIGNO UID CORE_SIZE_LIMIT", argv[0]);
     }
+
     openlog("abrt", LOG_PID, LOG_DAEMON);
     logmode = LOGMODE_SYSLOG;
 
@@ -105,8 +280,10 @@ int main(int argc, char** argv)
     bool setting_MakeCompatCore = false;
     parse_conf(CONF_DIR"/plugins/CCpp.conf", &setting_MaxCrashReportsSize, &setting_MakeCompatCore);
 
-    int core_fd = STDIN_FILENO;
-    off_t core_size = 0;
+    /* Open a fd to compat coredump, if requested and is possible */
+    int user_core_fd = -1;
+    if (setting_MakeCompatCore && ulimit_c != 0)
+        user_core_fd = open_user_core(user_pwd, uid, pid);
 
     const char *signame = NULL;
     /* Tried to use array for this but C++ does not support v[] = { [IDX] = "str" } */
@@ -122,10 +299,7 @@ int main(int argc, char** argv)
       //case SIGTRAP: signame = "TRAP"; break; //Trace/breakpoint trap
       //case SIGXCPU: signame = "XCPU"; break; //CPU time limit exceeded (4.2BSD)
       //case SIGXFSZ: signame = "XFSZ"; break; //File size limit exceeded (4.2BSD)
-    }
-    if (signame == NULL) {
-        /* not a signal we care about */
-        goto create_user_core;
+        default: goto create_user_core; // not a signal we care about
     }
 
     if (!daemon_is_ok())
@@ -151,7 +325,7 @@ int main(int argc, char** argv)
          * if they happen too often. Else, write new marker value.
          */
         snprintf(path, sizeof(path), "%s/last-ccpp", dddir);
-        fd = open(path, O_RDWR | O_CREAT, 0600);
+        int fd = open(path, O_RDWR | O_CREAT, 0600);
         if (fd >= 0)
         {
             int sz;
@@ -188,9 +362,9 @@ int main(int argc, char** argv)
              * Unlike dirs, mere files are ignored by abrtd.
              */
             snprintf(path, sizeof(path), "%s/abrtd-coredump", dddir);
-            core_fd = xopen3(path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
-            core_size = copyfd_eof(STDIN_FILENO, core_fd, COPYFD_SPARSE);
-            if (core_size < 0 || close(core_fd) != 0)
+            int abrt_core_fd = xopen3(path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+            off_t core_size = copyfd_eof(STDIN_FILENO, abrt_core_fd, COPYFD_SPARSE);
+            if (core_size < 0 || fsync(abrt_core_fd) != 0)
             {
                 unlink(path);
                 /* copyfd_eof logs the error including errno string,
@@ -201,58 +375,79 @@ int main(int argc, char** argv)
             return 0;
         }
 
-        char* cmdline = get_cmdline(pid); /* never NULL */
-        char *reason = xasprintf("Process %s was killed by signal %s (SIG%s)", executable, signal_str, signame ? signame : signal_str);
-
         unsigned path_len = snprintf(path, sizeof(path), "%s/ccpp-%ld-%lu.new",
                 dddir, (long)time(NULL), (long)pid);
-        if (path_len >= sizeof(path))
-            exit(1);
+        if (path_len >= (sizeof(path) - sizeof("/"FILENAME_COREDUMP)))
+            return 1;
 
         CDebugDump dd;
+        char *cmdline = get_cmdline(pid); /* never NULL */
+        char *reason = xasprintf("Process %s was killed by signal %s (SIG%s)", executable, signal_str, signame ? signame : signal_str);
         dd.Create(path, uid);
         dd.SaveText(FILENAME_ANALYZER, "CCpp");
         dd.SaveText(FILENAME_EXECUTABLE, executable);
         dd.SaveText(FILENAME_CMDLINE, cmdline);
         dd.SaveText(FILENAME_REASON, reason);
-
-        int len = strlen(path);
-        snprintf(path + len, sizeof(path) - len, "/"FILENAME_COREDUMP);
+        free(cmdline);
+        free(reason);
 
         /* We need coredumps to be readable by all, because
          * when abrt daemon processes coredump,
          * process producing backtrace is run under the same UID
          * as the crashed process.
          * Thus 644, not 600 */
-        core_fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0644);
-        if (core_fd < 0)
+        strcpy(path + path_len, "/"FILENAME_COREDUMP);
+        int abrt_core_fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0644);
+        if (abrt_core_fd < 0)
         {
             int sv_errno = errno;
             dd.Delete();
             dd.Close();
+            if (user_core_fd >= 0)
+            {
+                xchdir(user_pwd);
+                unlink(core_basename);
+            }
             errno = sv_errno;
             perror_msg_and_die("can't open '%s'", path);
         }
-//TODO: chown to uid:abrt?
+
+        /* We write both coredumps at once.
+         * We can't write user coredump first, since it might be truncated
+         * and thus can't be copied and used as abrt coredump;
+         * and if we write abrt coredump first and then copy it as user one,
+         * then we have a race when process exits but coredump does not exist yet:
+         * $ echo -e '#include<signal.h>\nmain(){raise(SIGSEGV);}' | gcc -o test -x c -
+         * $ rm -f core*; ulimit -c unlimited; ./test; ls -l core*
+         * 21631 Segmentation fault (core dumped) ./test
+         * ls: cannot access core*: No such file or directory <=== BAD
+         */
+//TODO: fchown abrt_core_fd to uid:abrt?
 //Currently it is owned by 0:0 but is readable by anyone, so the owner
 //of the crashed binary still can access it, as he has
 //r-x access to the dump dir.
-        core_size = copyfd_eof(STDIN_FILENO, core_fd, COPYFD_SPARSE);
-        if (core_size < 0 || fsync(core_fd) != 0)
+        off_t core_size = copyfd_sparse(STDIN_FILENO, abrt_core_fd, user_core_fd, ulimit_c);
+        if (core_size < 0 || fsync(abrt_core_fd) != 0)
         {
             unlink(path);
             dd.Delete();
             dd.Close();
-            /* copyfd_eof logs the error including errno string,
+            if (user_core_fd >= 0)
+            {
+                xchdir(user_pwd);
+                unlink(core_basename);
+            }
+            /* copyfd_sparse logs the error including errno string,
              * but it does not log file name */
-            error_msg_and_die("error saving coredump to %s", path);
+            error_msg_and_die("error writing %s", path);
         }
-        lseek(core_fd, 0, SEEK_SET);
-        /* note: core_fd is still open, we may use it later to copy core to user's dir */
         log("saved core dump of pid %lu (%s) to %s (%llu bytes)", (long)pid, executable, path, (long long)core_size);
-        free(executable);
-        free(cmdline);
-        path[len] = '\0'; /* path now contains directory name */
+        if (user_core_fd >= 0 && core_size >= ulimit_c)
+        {
+            /* user coredump is too big, nuke it */
+            xchdir(user_pwd);
+            unlink(core_basename);
+        }
 
         /* We close dumpdir before we start catering for crash storm case.
          * Otherwise, delete_debug_dump_dir's from other concurrent
@@ -261,6 +456,7 @@ int main(int argc, char** argv)
          * Classic deadlock.
          */
         dd.Close();
+        path[path_len] = '\0'; /* path now contains only directory name */
         char *newpath = xstrndup(path, path_len - (sizeof(".new")-1));
         if (rename(path, newpath) == 0)
             strcpy(path, newpath);
@@ -272,7 +468,7 @@ int main(int argc, char** argv)
             trim_debug_dumps(setting_MaxCrashReportsSize, path);
         }
 
-        /* fall through to creating user core */
+        return 0;
     }
     catch (CABRTException& e)
     {
@@ -283,108 +479,26 @@ int main(int argc, char** argv)
         error_msg_and_die("%s", e.what());
     }
 
-
+    /* We didn't create abrt dump, but may need to create compat coredump */
  create_user_core:
-    if (!setting_MakeCompatCore)
+    if (user_core_fd < 0)
         return 0;
 
-    /* note: core_size may be == 0 ("unknown") */
-    if (core_size > ulimit_c || ulimit_c == 0)
-        return 0;
-
-    /* Write a core file for user */
-
-    struct passwd* pw = getpwuid(uid);
-    gid_t gid = pw ? pw->pw_gid : uid;
-    setgroups(1, &gid);
-    xsetregid(gid, gid);
-    xsetreuid(uid, uid);
-
-    errno = 0;
-    if (user_pwd == NULL
-     || chdir(user_pwd) != 0
-    ) {
-        perror_msg_and_die("can't cd to %s", user_pwd);
-    }
-
-    /* Mimic "core.PID" if requested */
-    char core_basename[sizeof("core.%lu") + sizeof(long)*3] = "core";
-    char buf[] = "0\n";
-    fd = open("/proc/sys/kernel/core_uses_pid", O_RDONLY);
-    if (fd >= 0)
-    {
-        read(fd, buf, sizeof(buf));
-        close(fd);
-    }
-    if (strcmp(buf, "1\n") == 0)
-    {
-        sprintf(core_basename, "core.%lu", (long)pid);
-    }
-
-    /* man core:
-     * There are various circumstances in which a core dump file
-     * is not produced:
-     *
-     * [skipped obvious ones]
-     * The process does not have permission to write the core file.
-     * ...if a file with the same name exists and is not writable
-     * or is not a regular file (e.g., it is a directory or a symbolic link).
-     *
-     * A file with the same name already exists, but there is more
-     * than one hard link to that file.
-     *
-     * The file system where the core dump file would be created is full;
-     * or has run out of inodes; or is mounted read-only;
-     * or the user has reached their quota for the file system.
-     *
-     * The RLIMIT_CORE or RLIMIT_FSIZE resource limits for the process
-     * are set to zero.
-     * [shouldn't it be checked by kernel? 2.6.30.9-96 doesn't, still
-     * calls us even if "ulimit -c 0"]
-     *
-     * The binary being executed by the process does not have
-     * read permission enabled. [how we can check it here?]
-     *
-     * The process is executing a set-user-ID (set-group-ID) program
-     * that is owned by a user (group) other than the real
-     * user (group) ID of the process. [TODO?]
-     * (However, see the description of the prctl(2) PR_SET_DUMPABLE operation,
-     * and the description of the /proc/sys/fs/suid_dumpable file in proc(5).)
-     */
-
-    /* Do not O_TRUNC: if later checks fail, we do not want to have file already modified here */
-    errno = 0;
-    int usercore_fd = open(core_basename, O_WRONLY | O_CREAT | O_NOFOLLOW, 0600); /* kernel makes 0600 too */
-    if (usercore_fd < 0
-     || fstat(usercore_fd, &sb) != 0
-     || !S_ISREG(sb.st_mode)
-     || sb.st_nlink != 1
-    /* kernel internal dumper checks this too: if (inode->i_uid != current->fsuid) <fail>, need to mimic? */
-    ) {
-        perror_msg_and_die("%s/%s is not a regular file with link count 1", user_pwd, core_basename);
-    }
-
-    /* Note: we do not copy more than ulimit_c */
-    off_t size;
-    if (ftruncate(usercore_fd, 0) != 0
-     || (size = copyfd_size(core_fd, usercore_fd, ulimit_c, COPYFD_SPARSE)) < 0
-     || close(usercore_fd) != 0
-    ) {
+    off_t core_size = copyfd_size(STDIN_FILENO, user_core_fd, ulimit_c, COPYFD_SPARSE);
+    if (core_size < 0 || fsync(user_core_fd) != 0) {
         /* perror first, otherwise unlink may trash errno */
-        perror_msg("write error writing %s/%s", user_pwd, core_basename);
+        perror_msg("error writing %s/%s", user_pwd, core_basename);
+        xchdir(user_pwd);
         unlink(core_basename);
         return 1;
     }
-    if (size == ulimit_c && size != core_size)
+    if (core_size >= ulimit_c)
     {
-        /* We copied exactly ulimit_c bytes (and it doesn't accidentally match
-         * core_size (imagine exactly 1MB coredump with "ulimit -c 1M" - that'd be ok)),
-         * it means that core is larger than ulimit_c. Abort and delete the dump.
-         */
+        xchdir(user_pwd);
         unlink(core_basename);
         return 1;
     }
-    log("saved core dump of pid %lu to %s/%s (%llu bytes)", (long)pid, user_pwd, core_basename, (long long)size);
+    log("saved core dump of pid %lu to %s/%s (%llu bytes)", (long)pid, user_pwd, core_basename, (long long)core_size);
 
     return 0;
 }