Fix after security review

* File: src/shared/util.c
  Function: write_fd
  Write in function may return EINTR correctly. Function does  not
  support this case correctly. If the interrupt occurs program will not
  dump all data to file. The interrupt may come from child process
  (SIGCHILD).

* File: src/shared/util.c
  Function: fprintf_fd
  You may replace fprintf_fd with standard library function dprintf.

* File: src/shared/util.c
  Functions: dump_file_write_fd, copy_file, cat_file
  Functions don’t support EINTR correctly (write in function write_fd
  may be interrupt). In this functions it is not clear who will generate
  signal to interrupt write call, so this is not security risk now.  You
  may replace loop with standard library function sendfile.

* File: crash-manager.c
  Function: launch_crash_popup
  Memory leak: value conn was allocated but was not deallocated.

* File: crash-manager.c
  Function: prepare_paths
  Invalid strings are stored in crash_crash_path and crash_temp_path.
  Both strings are truncated by 1 byte.

Change-Id: Ie174b5ee043321861912f1b9233f0f6a6afd027e

diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c
index 419e16a..0a2bc23 100644 (file)
--- a/src/crash-manager/crash-manager.c
+++ b/src/crash-manager/crash-manager.c
@@ -235,7 +235,7 @@ static int prepare_paths(void)
                _E("Couldn't allocate memory for crash_crash_path: %m\n");
                return 0;
        }
-       snprintf(crash_crash_path, tmp_len, "%s%s", crash_root_path, CRASH_PATH_SUBDIR);
+       snprintf(crash_crash_path, tmp_len + 1, "%s%s", crash_root_path, CRASH_PATH_SUBDIR);
 
        tmp_len = strlen(crash_root_path) + strlen(CRASH_TEMP_SUBDIR);
        crash_temp_path = (char*)malloc(tmp_len + 1);
@@ -243,7 +243,7 @@ static int prepare_paths(void)
                _E("Couldn't allocate memory for crash_temp_path: %m\n");
                return 0;
        }
-       snprintf(crash_temp_path, tmp_len, "%s%s", crash_root_path, CRASH_TEMP_SUBDIR);
+       snprintf(crash_temp_path, tmp_len + 1, "%s%s", crash_root_path, CRASH_TEMP_SUBDIR);
        return 1;
 }
 
@@ -682,6 +682,8 @@ exit:
                g_variant_unref(reply);
        if (parameters)
                g_variant_unref(parameters);
+
+       g_object_unref(conn);
 }
 
 static int dump_system_state(const struct crash_info *cinfo)

diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c
index 1c5296e..776ec2b 100644 (file)
--- a/src/dump_systemstate/dump_systemstate.c
+++ b/src/dump_systemstate/dump_systemstate.c
@@ -157,7 +157,7 @@ int main(int argc, char *argv[])
 
        dpercent = get_disk_used_percent("/opt");
        if (90 < dpercent) {
-               fprintf_fd(out_fd, "\n==== System disk space usage detail - %d\% (/bin/du -ah /opt)\n", dpercent);
+               fprintf_fd(out_fd, "\n==== System disk space usage detail - %d%% (/bin/du -ah /opt)\n", dpercent);
                char *du_args[] = {"/bin/du", "-ah", "/opt", "--exclude=/opt/usr", NULL};
                ret = run_command_write_fd_timeout(du_args[0], du_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT);
                if (ret < 0)

diff --git a/src/shared/util.c b/src/shared/util.c
index 86e95e4..44f9685 100644 (file)
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -22,6 +22,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/time.h>
+#include <sys/sendfile.h>
 #include <dirent.h>
 #include <fcntl.h>
 #include <stdarg.h>
@@ -145,21 +146,6 @@ int system_command_with_timeout(int timeout_seconds, char *command)
        }
 }
 
-/* WARNING : formatted string buffer is limited to 1024 byte */
-int fprintf_fd(int fd, const char *fmt, ...)
-{
-       int n;
-       int ret;
-       char buff[1024];
-       va_list args;
-
-       va_start(args, fmt);
-       n = vsnprintf(buff, 1024 - 1, fmt, args);
-       ret = write(fd, buff, n);
-       va_end(args);
-       return ret;
-}
-
 int file_exist(const char *file)
 {
        FILE *fp;
@@ -179,6 +165,8 @@ int write_fd(int fd, const void *buf, int len)
        while (len) {
                count = write(fd, buf, len);
                if (count < 0) {
+                       if (errno == EINTR)
+                               continue;
                        if (total)
                                return total;
                        return count;
@@ -190,11 +178,26 @@ int write_fd(int fd, const void *buf, int len)
        return total;
 }
 
+static int copy_bytes(int dfd, int sfd)
+{
+       int res = 1;
+       ssize_t bytes_sent;
+       do
+               bytes_sent = sendfile(dfd, sfd, NULL, PIPE_BUF);
+       while (bytes_sent > 0);
+
+       if (bytes_sent == -1) {
+               _E("sendfile() error: %m\n");
+               res = -1;
+       }
+       return res;
+}
+
 int copy_file(char *src, char *dst)
 {
+       int res;
        int sfd;
        int dfd;
-       char buf[PIPE_BUF];
 
        if (!src || !dst) {
                _E("Invalid argument\n");
@@ -211,23 +214,19 @@ int copy_file(char *src, char *dst)
                _SE("Failed to open (%s)\n", dst);
                return -1;
        }
-       for (;;) {
-               int ret = read(sfd, buf, sizeof(buf));
-               if (ret > 0)
-                       ret = write_fd(dfd, buf, ret);
-               if (ret <= 0)
-                       break;
-       }
+
+       res = copy_bytes(dfd, sfd);
+
        close(sfd);
        close(dfd);
-       return 1;
+       return res;
 }
 
 int cat_file(char *src, char *dst)
 {
+       int res;
        int sfd;
        int dfd;
-       char buf[PIPE_BUF];
 
        if (!src || !dst) {
                _E("Invalid argument\n");
@@ -244,16 +243,12 @@ int cat_file(char *src, char *dst)
                _SE("Failed to open (%s)\n", dst);
                return -1;
        }
-       for (;;) {
-               int ret = read(sfd, buf, sizeof(buf));
-               if (ret > 0)
-                       ret = write_fd(dfd, buf, ret);
-               if (ret <= 0)
-                       break;
-       }
+
+       res = copy_bytes(dfd, sfd);
+
        close(sfd);
        close(dfd);
-       return 1;
+       return res;
 }
 
 int move_file(char *src, char *dst)
@@ -267,8 +262,8 @@ int move_file(char *src, char *dst)
 
 int dump_file_write_fd(char *src, int dfd)
 {
+       int res;
        int sfd;
-       char buf[PIPE_BUF];
 
        if (!src) {
                _E("Invalid argument\n");
@@ -279,15 +274,11 @@ int dump_file_write_fd(char *src, int dfd)
                _SE("Failed to open (%s)\n", src);
                return -1;
        }
-       for (;;) {
-               int ret = read(sfd, buf, sizeof(buf));
-               if (ret > 0)
-                       ret = write_fd(dfd, buf, ret);
-               if (ret <= 0)
-                       break;
-       }
+
+       res = copy_bytes(dfd, sfd);
+
        close(sfd);
-       return 1;
+       return res;
 }
 
 static int run_command(char *path, char *args[], char *env[], int fd[])

diff --git a/src/shared/util.h b/src/shared/util.h
index e0978ac..544fe11 100644 (file)
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -31,7 +31,7 @@ int system_command_parallel(char *command);
 
 int wait_system_command(int pid);
 
-int fprintf_fd(int fd, const char *fmt, ...);
+#define fprintf_fd(fd, fmt, ...) dprintf(fd, fmt, ##__VA_ARGS__)
 
 int write_fd(int fd, const void *buf, int len);
 

diff --git a/src/sys-assert/util.c b/src/sys-assert/util.c
index 30200ba..3fc2e66 100644 (file)
--- a/src/sys-assert/util.c
+++ b/src/sys-assert/util.c
@@ -61,22 +61,6 @@ char *fgets_fd(char *str, int len, int fd)
        return (num == 0 && cs == str) ? NULL : str;
 }
 
-/* WARNING : formatted string buffer is limited to 1024 byte */
-int fprintf_fd(int fd, const char *fmt, ...)
-{
-       int n, ret;
-       char buff[1024];
-       va_list args;
-
-       va_start(args, fmt);
-       n = vsnprintf(buff, 1024 - 1, fmt, args);
-       ret = write(fd, buff, n);
-       if (ret < 0)
-               fprintf(stderr, "write failed\n");
-       va_end(args);
-       return n;
-}
-
 char *remove_path(const char *cmd)
 {
        char *cp;

diff --git a/src/sys-assert/util.h b/src/sys-assert/util.h
index a06077e..ff7f7f3 100644 (file)
--- a/src/sys-assert/util.h
+++ b/src/sys-assert/util.h
@@ -21,8 +21,7 @@ int open_read(const char *path, char *buf, int size);
 
 char *fgets_fd(char *str, int len, int fd);
 
-/* WARNING : formatted string buffer is limited to 1024 byte */
-int fprintf_fd(int fd, const char *fmt, ...);
+#define fprintf_fd(fd, fmt, ...) dprintf(fd, fmt, ##__VA_ARGS__)
 
 char *remove_path(const char *cmd);