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 419e16ac686667bfdb31d00c4f21cf37af06728c..0a2bc2393060acb85bbd570a587073c114acae58 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 1c5296ee5e7bfdbda2949b95ae24e54be4d6ecef..776ec2b2b0b4245106ca98d00e2445d7e605fe2b 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 86e95e452698e9e1df09836f71713da17a6ebade..44f9685b4f37dbb6a7aa49ebeee3efa1c2440888 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 e0978acd7f6c8e9d0766f351436bde00e716f59a..544fe1167ebb6eb8cad75c278066ab07dee2a730 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 30200bafca773fe20ba1a69689768b844c2715cc..3fc2e6605581b5d074ba31b55de26ac0fd880f5d 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 a06077e2c809ebf9d0961cbcbce9c67d93e1426c..ff7f7f3a9d9c3d40708227400cdc96fc49a456a6 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);