From ae933d0d03982932d7adaf88af829f5c5c8e374d Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 11:52:33 +0200 Subject: [PATCH 01/11] conmon: Handle EINTR and partial writes when writing Any write could be interupted by EINTR if we get some kind of signal, which means we could be either reporting a EINTR error or a partial write (if some data was written). Its also generally good to handle partial writes correctly, as they can happen e.g. when writing to full pipes. Signed-off-by: Alexander Larsson --- conmon/conmon.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/conmon/conmon.c b/conmon/conmon.c index 25c682a5..9767876a 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -109,6 +109,27 @@ static GOptionEntry entries[] = #define CGROUP_ROOT "/sys/fs/cgroup" +static ssize_t write_all(int fd, const void *buf, size_t count) +{ + size_t remaining = count; + const char *p = buf; + ssize_t res; + + while (remaining > 0) { + do { + res = write(fd, p, remaining); + } while (res == -1 && errno == EINTR); + + if (res <= 0) + return -1; + + remaining -= res; + p += res; + } + + return count; +} + int set_k8s_timestamp(char *buf, ssize_t buflen) { struct tm *tm; @@ -221,7 +242,7 @@ int write_k8s_log(int fd, stdpipe_t pipe, const char *buf, ssize_t buflen) } /* Output the actual contents. */ - if (write(fd, buf, line_len) < 0) { + if (write_all(fd, buf, line_len) < 0) { nwarn("failed to write buffer to log"); goto next; } @@ -559,7 +580,7 @@ int main(int argc, char *argv[]) * We send -1 as pid to signal to parent that create container has failed. */ len = snprintf(buf, BUF_SIZE, "{\"pid\": %d}\n", -1); - if (len < 0 || write(sync_pipe_fd, buf, len) != len) { + if (len < 0 || write_all(sync_pipe_fd, buf, len) != len) { pexit("unable to send container pid to parent"); } } else { @@ -588,7 +609,7 @@ int main(int argc, char *argv[]) g_object_set(generator, "pretty", FALSE, NULL); data = json_generator_to_data (generator, &len); fprintf(stderr, "%s\n", data); - if (write(sync_pipe_fd, data, len) != (int)len) { + if (write_all(sync_pipe_fd, data, len) != (int)len) { ninfo("Unable to send container stderr message to parent"); } @@ -615,7 +636,7 @@ int main(int argc, char *argv[]) /* Send the container pid back to parent */ if (sync_pipe_fd > 0 && !exec) { len = snprintf(buf, BUF_SIZE, "{\"pid\": %d}\n", cpid); - if (len < 0 || write(sync_pipe_fd, buf, len) != len) { + if (len < 0 || write_all(sync_pipe_fd, buf, len) != len) { pexit("unable to send container pid to parent"); } } @@ -643,7 +664,7 @@ int main(int argc, char *argv[]) pexit("Failed to create eventfd"); wb = snprintf(buf, BUF_SIZE, "%d %d", efd, ofd); - if (write(cfd, buf, wb) < 0) + if (write_all(cfd, buf, wb) < 0) pexit("Failed to write to cgroup.event_control"); } @@ -749,7 +770,7 @@ out: /* Send the command exec exit code back to the parent */ if (sync_pipe_fd > 0) { len = snprintf(buf, BUF_SIZE, "{\"exit_code\": %d}\n", exit_status); - if (len < 0 || write(sync_pipe_fd, buf, len) != len) { + if (len < 0 || write_all(sync_pipe_fd, buf, len) != len) { pexit("unable to send exit status"); exit(1); } @@ -767,7 +788,7 @@ out: * to the parent. */ len = snprintf(buf, BUF_SIZE, "{\"exit_code\": %d}\n", WEXITSTATUS(runtime_status)); - if (len < 0 || write(sync_pipe_fd, buf, len) != len) { + if (len < 0 || write_all(sync_pipe_fd, buf, len) != len) { pexit("unable to send exit status"); exit(1); } From d34c5829f8a4d25b391ac7daa541de814f4c9d71 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 13:37:53 +0200 Subject: [PATCH 02/11] conmon: Write log in larger chunks Rather than writing the logs with one write per line, use writev() to write multiple lines in one call. Additionally, this avoids using dprintf() when writing to the log, which is nice because that doesn't correctly handle partial writes or ENOINTR. This also changes set_k8s_timestamp to add the pipe to the reused buffer so that we don't have to append it on each line. Signed-off-by: Alexander Larsson --- conmon/conmon.c | 92 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 80 insertions(+), 12 deletions(-) diff --git a/conmon/conmon.c b/conmon/conmon.c index 9767876a..a5a2329e 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -104,8 +104,8 @@ static GOptionEntry entries[] = { NULL } }; -/* strlen("1997-03-25T13:20:42.999999999+01:00") + 1 */ -#define TSBUFLEN 36 +/* strlen("1997-03-25T13:20:42.999999999+01:00 stdout ") + 1 */ +#define TSBUFLEN 44 #define CGROUP_ROOT "/sys/fs/cgroup" @@ -130,7 +130,72 @@ static ssize_t write_all(int fd, const void *buf, size_t count) return count; } -int set_k8s_timestamp(char *buf, ssize_t buflen) +#define WRITEV_BUFFER_N_IOV 128 + +typedef struct { + int iovcnt; + struct iovec iov[WRITEV_BUFFER_N_IOV]; +} writev_buffer_t; + +static ssize_t writev_buffer_flush (int fd, writev_buffer_t *buf) +{ + size_t count = 0; + ssize_t res; + struct iovec *iov; + int iovcnt; + + iovcnt = buf->iovcnt; + iov = buf->iov; + + while (iovcnt > 0) { + do { + res = writev(fd, iov, iovcnt); + } while (res == -1 && errno == EINTR); + + if (res <= 0) + return -1; + + count += res; + + while (res > 0) { + size_t from_this = MIN((size_t)res, iov->iov_len); + iov->iov_len -= from_this; + res -= from_this; + + if (iov->iov_len == 0) { + iov++; + iovcnt--; + } + } + } + + buf->iovcnt = 0; + + return count; +} + +ssize_t writev_buffer_append_segment(int fd, writev_buffer_t *buf, const void *data, ssize_t len) +{ + if (data == NULL) + return 1; + + if (len < 0) + len = strlen ((char *)data); + + if (buf->iovcnt == WRITEV_BUFFER_N_IOV && + writev_buffer_flush (fd, buf) < 0) + return -1; + + if (len > 0) { + buf->iov[buf->iovcnt].iov_base = (void *)data; + buf->iov[buf->iovcnt].iov_len = (size_t)len; + buf->iovcnt++; + } + + return 1; +} + +int set_k8s_timestamp(char *buf, ssize_t buflen, const char *pipename) { struct tm *tm; struct timespec ts; @@ -156,10 +221,10 @@ int set_k8s_timestamp(char *buf, ssize_t buflen) off = -off; } - len = snprintf(buf, buflen, "%d-%02d-%02dT%02d:%02d:%02d.%09ld%c%02d:%02d", + len = snprintf(buf, buflen, "%d-%02d-%02dT%02d:%02d:%02d.%09ld%c%02d:%02d %s ", tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec, ts.tv_nsec, - off_sign, off / 3600, off % 3600); + off_sign, off / 3600, off % 3600, pipename); if (len < buflen) err = 0; @@ -198,13 +263,14 @@ int write_k8s_log(int fd, stdpipe_t pipe, const char *buf, ssize_t buflen) { char tsbuf[TSBUFLEN]; static stdpipe_t trailing_line = NO_PIPE; + writev_buffer_t bufv = {0}; /* * Use the same timestamp for every line of the log in this buffer. * There is no practical difference in the output since write(2) is * fast. */ - if (set_k8s_timestamp(tsbuf, TSBUFLEN)) + if (set_k8s_timestamp(tsbuf, sizeof tsbuf, stdpipe_name(pipe))) /* TODO: We should handle failures much more cleanly than this. */ return -1; @@ -231,18 +297,16 @@ int write_k8s_log(int fd, stdpipe_t pipe, const char *buf, ssize_t buflen) * wasn't one output) but without modifying the file in a * non-append-only way there's not much we can do. */ - char *leading = ""; - if (trailing_line != NO_PIPE) - leading = "\n"; - - if (dprintf(fd, "%s%s %s ", leading, tsbuf, stdpipe_name(pipe)) < 0) { + if ((trailing_line != NO_PIPE && + writev_buffer_append_segment(fd, &bufv, "\n", -1) < 0) || + writev_buffer_append_segment(fd, &bufv, tsbuf, -1) < 0) { nwarn("failed to write (timestamp, stream) to log"); goto next; } } /* Output the actual contents. */ - if (write_all(fd, buf, line_len) < 0) { + if (writev_buffer_append_segment(fd, &bufv, buf, line_len) < 0) { nwarn("failed to write buffer to log"); goto next; } @@ -256,6 +320,10 @@ next: buflen -= line_len; } + if (writev_buffer_flush (fd, &bufv) < 0) { + nwarn("failed to flush buffer to log"); + } + return 0; } From fe80f857ca2a5e141f1ad222d623b09680da5556 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 14:02:38 +0200 Subject: [PATCH 03/11] conmon: Fix cgroup subsystem parsing The code as is doesn't handle merged controllers. For instance, I have this in my /proc/self/cgrous: 4:cpu,cpuacct:/user.slice/user-0.slice/session-4.scope The current code fails to match "cpuacct" wit this line, and additionally it just does a prefix match so if you were looking for say "cpu", it would match this: 2:cpuset:/ I also removed some ninfo spew that didn't seem very useful. Signed-off-by: Alexander Larsson --- conmon/conmon.c | 55 ++++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/conmon/conmon.c b/conmon/conmon.c index a5a2329e..fdb36845 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -72,10 +72,17 @@ static inline void gstring_free_cleanup(GString **string) g_string_free(*string, TRUE); } +static inline void strv_cleanup(char ***strv) +{ + if (strv) + g_strfreev (*strv); +} + #define _cleanup_free_ _cleanup_(freep) #define _cleanup_close_ _cleanup_(closep) #define _cleanup_fclose_ _cleanup_(fclosep) #define _cleanup_gstring_ _cleanup_(gstring_free_cleanup) +#define _cleanup_strv_ _cleanup_(strv_cleanup) #define BUF_SIZE 256 #define CMD_SIZE 1024 @@ -350,37 +357,43 @@ static char *process_cgroup_subsystem_path(int pid, const char *subsystem) { _cleanup_free_ char *line = NULL; ssize_t read; size_t len = 0; - char *ptr; + char *ptr, *path; char *subsystem_path = NULL; + int i; while ((read = getline(&line, &len, fp)) != -1) { + _cleanup_strv_ char **subsystems = NULL; ptr = strchr(line, ':'); if (ptr == NULL) { nwarn("Error parsing cgroup, ':' not found: %s", line); return NULL; } ptr++; - if (!strncmp(ptr, subsystem, strlen(subsystem))) { - char *path = strchr(ptr, '/'); - if (path == NULL) { - nwarn("Error finding path in cgroup: %s", line); - return NULL; - } - ninfo("PATH: %s", path); - const char *subpath = strchr(subsystem, '='); - if (subpath == NULL) { - subpath = subsystem; - } else { - subpath++; - } + path = strchr(ptr, ':'); + if (path == NULL) { + nwarn("Error parsing cgroup, second ':' not found: %s", line); + return NULL; + } + *path = 0; + path++; + subsystems = g_strsplit (ptr, ",", -1); + for (i = 0; subsystems[i] != NULL; i++) { + if (strcmp (subsystems[i], subsystem) == 0) { + char *subpath = strchr(subsystems[i], '='); + if (subpath == NULL) { + subpath = ptr; + } else { + *subpath = 0; + } - rc = asprintf(&subsystem_path, "%s/%s%s", CGROUP_ROOT, subpath, path); - if (rc < 0) { - nwarn("Failed to allocate memory for subsystemd path"); - return NULL; + rc = asprintf(&subsystem_path, "%s/%s%s", CGROUP_ROOT, subpath, path); + if (rc < 0) { + nwarn("Failed to allocate memory for subsystemd path"); + return NULL; + } + + subsystem_path[strlen(subsystem_path) - 1] = '\0'; + return subsystem_path; } - ninfo("SUBSYSTEM_PATH: %s", subsystem_path); - subsystem_path[strlen(subsystem_path) - 1] = '\0'; - return subsystem_path; } } From d2f09ef4830437beee67004c721e2bb09cf1e45e Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 14:15:50 +0200 Subject: [PATCH 04/11] conmon: Increase buffer size The buffer is used to read from the stderr/stdout stream, which can easily be larger than 256 bytes. With a larger buffer we will do fewer, larger reads, which is more efficient. And 8k more stack size use is not really a problem. Signed-off-by: Alexander Larsson --- conmon/conmon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conmon/conmon.c b/conmon/conmon.c index fdb36845..78d9367d 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -84,7 +84,7 @@ static inline void strv_cleanup(char ***strv) #define _cleanup_gstring_ _cleanup_(gstring_free_cleanup) #define _cleanup_strv_ _cleanup_(strv_cleanup) -#define BUF_SIZE 256 +#define BUF_SIZE 8192 #define CMD_SIZE 1024 #define MAX_EVENTS 10 From 829ec7f351e1f26eb4c573e130a8ab64bfcf827d Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 14:27:00 +0200 Subject: [PATCH 05/11] conmon: Build argv instead of commandline to spawn runtime This means we don't have to spawn via a shell, but it also means we do the right thing for any input that would have needed to be escaped. For instance if the container name had a $ in i, or even worse, a back-quote! Signed-off-by: Alexander Larsson --- conmon/conmon.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/conmon/conmon.c b/conmon/conmon.c index 78d9367d..ecd0068b 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -429,7 +429,7 @@ int main(int argc, char *argv[]) int num_stdio_fds = 0; GError *error = NULL; GOptionContext *context; - _cleanup_gstring_ GString *cmd = NULL; + GPtrArray *runtime_argv = NULL; /* Used for OOM notification API */ _cleanup_close_ int efd = -1; @@ -552,27 +552,40 @@ int main(int argc, char *argv[]) slavefd_stderr = fds[1]; } - cmd = g_string_new(runtime_path); + runtime_argv = g_ptr_array_new(); + g_ptr_array_add(runtime_argv, runtime_path); /* Generate the cmdline. */ if (!exec && systemd_cgroup) - g_string_append_printf(cmd, " --systemd-cgroup"); + g_ptr_array_add(runtime_argv, "--systemd-cgroup"); - if (exec) - g_string_append_printf(cmd, " exec -d --pid-file %s", pid_file); - else - g_string_append_printf(cmd, " create --bundle %s --pid-file %s", bundle_path, pid_file); + if (exec) { + g_ptr_array_add (runtime_argv, "exec"); + g_ptr_array_add (runtime_argv, "-d"); + g_ptr_array_add (runtime_argv, "--pid-file"); + g_ptr_array_add (runtime_argv, pid_file); + } else { + g_ptr_array_add (runtime_argv, "create"); + g_ptr_array_add (runtime_argv, "--bundle"); + g_ptr_array_add (runtime_argv, bundle_path); + g_ptr_array_add (runtime_argv, "--pid-file"); + g_ptr_array_add (runtime_argv, pid_file); + } - if (terminal) - g_string_append_printf(cmd, " --console-socket %s", csname); + if (terminal) { + g_ptr_array_add(runtime_argv, "--console-socket"); + g_ptr_array_add(runtime_argv, csname); + } /* Set the exec arguments. */ if (exec) { - g_string_append_printf(cmd, " --process %s", exec_process_spec); + g_ptr_array_add(runtime_argv, "--process"); + g_ptr_array_add(runtime_argv, exec_process_spec); } /* Container name comes last. */ - g_string_append_printf(cmd, " %s", cid); + g_ptr_array_add(runtime_argv, cid); + g_ptr_array_add(runtime_argv, NULL); /* * We have to fork here because the current runC API dups the stdio of the @@ -587,8 +600,6 @@ int main(int argc, char *argv[]) if (create_pid < 0) { pexit("Failed to fork the create command"); } else if (!create_pid) { - char *argv[] = {"sh", "-c", cmd->str, NULL}; - /* We only need to touch the stdio if we have terminal=false. */ /* FIXME: This results in us not outputting runc error messages to crio's log. */ if (slavefd_stdout >= 0) { @@ -600,11 +611,12 @@ int main(int argc, char *argv[]) pexit("Failed to dup over stderr"); } - /* Exec into the process. TODO: Don't use the shell. */ - execv("/bin/sh", argv); + execv(g_ptr_array_index(runtime_argv,0), (char **)runtime_argv->pdata); exit(127); } + g_ptr_array_free (runtime_argv, TRUE); + /* The runtime has that fd now. We don't need to touch it anymore. */ close(slavefd_stdout); close(slavefd_stderr); From f3408cbb5c696985fe9c6b3b6655c227c154bbce Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 14:48:00 +0200 Subject: [PATCH 06/11] conmon: Make all file descriptors CLOEXEC We want to avoid inheriting these into the child. Doing so is both confusing for the child, and a potential security issue if the container has access to FDs that are from the outside of the container. Some of these are created after we fork for the child, so they are not technically necessary. However, its best to do this as we may change the code in the future and forget about this. Signed-off-by: Alexander Larsson --- conmon/conmon.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/conmon/conmon.c b/conmon/conmon.c index ecd0068b..85c23681 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -348,7 +348,7 @@ static char *process_cgroup_subsystem_path(int pid, const char *subsystem) { } _cleanup_fclose_ FILE *fp = NULL; - fp = fopen(cgroups_file_path, "r"); + fp = fopen(cgroups_file_path, "re"); if (fp == NULL) { nwarn("Failed to open cgroups file: %s", cgroups_file_path); return NULL; @@ -482,10 +482,12 @@ int main(int argc, char *argv[]) sync_pipe_fd = strtol(sync_pipe, &endptr, 10); if (errno != 0 || *endptr != '\0') pexit("unable to parse _OCI_SYNCPIPE"); + if (fcntl(sync_pipe_fd, F_SETFD, FD_CLOEXEC) == -1) + pexit("unable to make _OCI_SYNCPIPE CLOEXEC"); } /* Open the log path file. */ - logfd = open(log_path, O_WRONLY | O_APPEND | O_CREAT); + logfd = open(log_path, O_WRONLY | O_APPEND | O_CREAT | O_CLOEXEC); if (logfd < 0) pexit("Failed to open log file"); @@ -539,13 +541,13 @@ int main(int argc, char *argv[]) * used anything else (and it wouldn't be a good idea to create a new * pty pair in the host). */ - if (pipe(fds) < 0) + if (pipe2(fds, O_CLOEXEC) < 0) pexit("Failed to create !terminal stdout pipe"); masterfd_stdout = fds[0]; slavefd_stdout = fds[1]; - if (pipe(fds) < 0) + if (pipe2(fds, O_CLOEXEC) < 0) pexit("Failed to create !terminal stderr pipe"); masterfd_stderr = fds[0]; @@ -743,17 +745,17 @@ int main(int argc, char *argv[]) bool oom_handling_enabled = true; char memory_cgroup_file_path[PATH_MAX]; snprintf(memory_cgroup_file_path, PATH_MAX, "%s/cgroup.event_control", memory_cgroup_path); - if ((cfd = open(memory_cgroup_file_path, O_WRONLY)) == -1) { + if ((cfd = open(memory_cgroup_file_path, O_WRONLY | O_CLOEXEC)) == -1) { nwarn("Failed to open %s", memory_cgroup_file_path); oom_handling_enabled = false; } if (oom_handling_enabled) { snprintf(memory_cgroup_file_path, PATH_MAX, "%s/memory.oom_control", memory_cgroup_path); - if ((ofd = open(memory_cgroup_file_path, O_RDONLY)) == -1) + if ((ofd = open(memory_cgroup_file_path, O_RDONLY | O_CLOEXEC)) == -1) pexit("Failed to open %s", memory_cgroup_file_path); - if ((efd = eventfd(0, 0)) == -1) + if ((efd = eventfd(0, EFD_CLOEXEC)) == -1) pexit("Failed to create eventfd"); wb = snprintf(buf, BUF_SIZE, "%d %d", efd, ofd); @@ -767,7 +769,7 @@ int main(int argc, char *argv[]) * attach and other important things. Using epoll directly is just * really nasty. */ - epfd = epoll_create(5); + epfd = epoll_create1(EPOLL_CLOEXEC); if (epfd < 0) pexit("epoll_create"); ev.events = EPOLLIN; From 1a168cb196f626389fd99e2b205c7eedad634616 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 15:19:15 +0200 Subject: [PATCH 07/11] conmon: Drop json-glib dependency json-glib is a fine library for parsing json. However, all we need to do is generate some trivial json output, so it is not needed. Signed-off-by: Alexander Larsson --- conmon/Makefile | 2 +- conmon/conmon.c | 57 +++++++++++++++++++++++++++++-------------------- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/conmon/Makefile b/conmon/Makefile index 425cf525..034ba59a 100644 --- a/conmon/Makefile +++ b/conmon/Makefile @@ -2,7 +2,7 @@ src = $(wildcard *.c) obj = $(src:.c=.o) override LIBS += $(shell pkg-config --libs glib-2.0 json-glib-1.0) -override CFLAGS += -std=c99 -Wall -Wextra $(shell pkg-config --cflags glib-2.0 json-glib-1.0) +override CFLAGS += -std=c99 -Wall -Wextra $(shell pkg-config --cflags glib-2.0) conmon: $(obj) $(CC) -o $@ $^ $(CFLAGS) $(LIBS) diff --git a/conmon/conmon.c b/conmon/conmon.c index 85c23681..9ece926d 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -18,7 +18,6 @@ #include #include -#include #include "cmsg.h" @@ -400,6 +399,34 @@ static char *process_cgroup_subsystem_path(int pid, const char *subsystem) { return NULL; } +static char *escape_json_string(const char *str) +{ + GString *escaped; + const char *p; + + p = str; + escaped = g_string_sized_new(strlen(str)); + + while (*p != 0) { + char c = *p++; + if (c == '\\' || c == '"') { + g_string_append_c(escaped, '\\'); + g_string_append_c(escaped, c); + } else if (c == '\n') { + g_string_append_printf (escaped, "\\n"); + } else if (c == '\t') { + g_string_append_printf (escaped, "\\t"); + } else if ((c > 0 && c < 0x1f) || c == 0x7f) { + g_string_append_printf (escaped, "\\u00%02x", (guint)c); + } else { + g_string_append_c (escaped, c); + } + } + + return g_string_free (escaped, FALSE); +} + + int main(int argc, char *argv[]) { int ret, runtime_status; @@ -685,32 +712,16 @@ int main(int argc, char *argv[]) */ num_read = read(masterfd_stderr, buf, BUF_SIZE); if (num_read > 0) { + _cleanup_free_ char *escaped_message = NULL; + ssize_t len; + buf[num_read] = '\0'; - JsonGenerator *generator = json_generator_new(); - JsonNode *root; - JsonObject *object; - gchar *data; - gsize len; + escaped_message = escape_json_string(buf); - root = json_node_new(JSON_NODE_OBJECT); - object = json_object_new(); - - json_object_set_int_member(object, "pid", -1); - json_object_set_string_member(object, "message", buf); - - json_node_take_object(root, object); - json_generator_set_root(generator, root); - - g_object_set(generator, "pretty", FALSE, NULL); - data = json_generator_to_data (generator, &len); - fprintf(stderr, "%s\n", data); - if (write_all(sync_pipe_fd, data, len) != (int)len) { + len = snprintf(buf, BUF_SIZE, "{\"pid\": %d, \"message\": \"%s\"}\n", -1, escaped_message); + if (len < 0 || write_all(sync_pipe_fd, buf, len) != len) { ninfo("Unable to send container stderr message to parent"); } - - g_free(data); - json_node_free(root); - g_object_unref(generator); } } } From fe6f1f47863aedda4cde3a63039d10def4321e0d Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 15:22:24 +0200 Subject: [PATCH 08/11] conmon: Add -Os flag This is what the other C code uses, and its nice to have as adding any optimization flags enables a bunch of more warnings. Signed-off-by: Alexander Larsson --- conmon/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conmon/Makefile b/conmon/Makefile index 034ba59a..4be94283 100644 --- a/conmon/Makefile +++ b/conmon/Makefile @@ -2,7 +2,7 @@ src = $(wildcard *.c) obj = $(src:.c=.o) override LIBS += $(shell pkg-config --libs glib-2.0 json-glib-1.0) -override CFLAGS += -std=c99 -Wall -Wextra $(shell pkg-config --cflags glib-2.0) +override CFLAGS += -std=c99 -Os -Wall -Wextra $(shell pkg-config --cflags glib-2.0) conmon: $(obj) $(CC) -o $@ $^ $(CFLAGS) $(LIBS) From f1b0f542e1e4817cbd188c5964e18bf149a5bfc9 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 15:23:11 +0200 Subject: [PATCH 09/11] conmon: Silence uninitialized read compiler warning This is not actually read uninitialized, its just that the compiler can't detect this, but we initilize it anyway to silence the compiler. Signed-off-by: Alexander Larsson --- conmon/conmon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conmon/conmon.c b/conmon/conmon.c index 9ece926d..d0fd9a90 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -813,7 +813,7 @@ int main(int argc, char *argv[]) for (int i = 0; i < ready; i++) { if (evlist[i].events & EPOLLIN) { int masterfd = evlist[i].data.fd; - stdpipe_t pipe; + stdpipe_t pipe = NO_PIPE; if (masterfd == masterfd_stdout) pipe = STDOUT_PIPE; else if (masterfd == masterfd_stderr) From f4b3e90141cdbfbc4f9071953080bb2566b50024 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 15:31:43 +0200 Subject: [PATCH 10/11] conmon: Make console socket mode 0700 It doesn't make sense for other users to connect to this, so lets make sure of this. Signed-off-by: Alexander Larsson --- conmon/conmon.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/conmon/conmon.c b/conmon/conmon.c index d0fd9a90..733292d1 100644 --- a/conmon/conmon.c +++ b/conmon/conmon.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -549,6 +550,8 @@ int main(int argc, char *argv[]) csfd = socket(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); if (csfd < 0) pexit("Failed to create console-socket"); + if (fchmod(csfd, 0700)) + pexit("Failed to change console-socket permissions"); /* XXX: This should be handled with a rename(2). */ if (unlink(csname) < 0) pexit("Failed to unlink temporary ranom path"); From 2507ba64536b4afab525303782ac2939c68c8dbf Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 2 Jun 2017 16:18:27 +0200 Subject: [PATCH 11/11] Remove json-glib in the remaining places Signed-off-by: Alexander Larsson --- Dockerfile | 1 - README.md | 2 -- conmon/Makefile | 2 +- contrib/test/crio-integration-playbook.yaml | 2 -- 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/Dockerfile b/Dockerfile index 1bcfca88..99b0d343 100644 --- a/Dockerfile +++ b/Dockerfile @@ -19,7 +19,6 @@ RUN apt-get update && apt-get install -y \ protobuf-compiler \ python-minimal \ libglib2.0-dev \ - libjson-glib-dev \ libapparmor-dev \ btrfs-tools \ libdevmapper1.02.1 \ diff --git a/README.md b/README.md index 7365762c..3f4fdf5d 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,6 @@ yum install -y \ libseccomp-devel \ libselinux-devel \ pkgconfig \ - json-glib-devel \ runc ``` @@ -79,7 +78,6 @@ apt install -y \ libseccomp-dev \ libselinux1-dev \ pkg-config \ - libjson-glib-dev \ runc ``` diff --git a/conmon/Makefile b/conmon/Makefile index 4be94283..460c1faa 100644 --- a/conmon/Makefile +++ b/conmon/Makefile @@ -1,7 +1,7 @@ src = $(wildcard *.c) obj = $(src:.c=.o) -override LIBS += $(shell pkg-config --libs glib-2.0 json-glib-1.0) +override LIBS += $(shell pkg-config --libs glib-2.0) override CFLAGS += -std=c99 -Os -Wall -Wextra $(shell pkg-config --cflags glib-2.0) conmon: $(obj) diff --git a/contrib/test/crio-integration-playbook.yaml b/contrib/test/crio-integration-playbook.yaml index f078d2aa..676c5c88 100644 --- a/contrib/test/crio-integration-playbook.yaml +++ b/contrib/test/crio-integration-playbook.yaml @@ -44,7 +44,6 @@ - libassuan-devel - libgpg-error-devel - pkgconfig - - json-glib-devel - skopeo-containers async: 600 poll: 10 @@ -71,7 +70,6 @@ - libassuan-devel - libgpg-error-devel - pkgconfig - - json-glib-devel - skopeo-containers async: 600 poll: 10