From fb44a233a5be72d8d1cfd02e300db7de2b4bf428 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Fri, 20 Feb 2015 16:32:58 +0100 Subject: coverity/13777: String not null terminated: RI. Problem : String not null terminated @ 1543. Diagnostic : Real issue. Rationale : We are reading a struct block0, which contains some string fields, from a file, without checking for string fields to be correctly terminated. That could cause a buffer overrun if file has somehow been garbled. Resolution : Add string fields check for nul termination. Mark issue as intentional (there seems to be no way of teaching coverity about read_eintr being ok that way). Helped-by: oni-link --- src/nvim/fileio.c | 4 ++-- src/nvim/memline.c | 12 ++++++++++++ src/nvim/vim.h | 7 ++----- 3 files changed, 16 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c index 9d4c990f3a..799a6a2a50 100644 --- a/src/nvim/fileio.c +++ b/src/nvim/fileio.c @@ -7416,7 +7416,7 @@ long read_eintr(int fd, void *buf, size_t bufsize) long ret; for (;; ) { - ret = vim_read(fd, buf, bufsize); + ret = read(fd, buf, bufsize); if (ret >= 0 || errno != EINTR) break; } @@ -7435,7 +7435,7 @@ long write_eintr(int fd, void *buf, size_t bufsize) /* Repeat the write() so long it didn't fail, other than being interrupted * by a signal. */ while (ret < (long)bufsize) { - wlen = vim_write(fd, (char *)buf + ret, bufsize - ret); + wlen = write(fd, (char *)buf + ret, bufsize - ret); if (wlen < 0) { if (errno != EINTR) break; diff --git a/src/nvim/memline.c b/src/nvim/memline.c index 5a577c6378..d6d7d3db1a 100644 --- a/src/nvim/memline.c +++ b/src/nvim/memline.c @@ -54,6 +54,7 @@ #include "nvim/cursor.h" #include "nvim/eval.h" #include "nvim/fileio.h" +#include "nvim/func_attr.h" #include "nvim/main.h" #include "nvim/mark.h" #include "nvim/mbyte.h" @@ -630,6 +631,15 @@ static int ml_check_b0_id(ZERO_BL *b0p) return OK; } +/// Return true if all strings in b0 are correct (nul-terminated). +static bool ml_check_b0_strings(ZERO_BL *b0p) FUNC_ATTR_NONNULL_ALL +{ + return (memchr(b0p->b0_version, NUL, 10) + && memchr(b0p->b0_uname, NUL, B0_UNAME_SIZE) + && memchr(b0p->b0_hname, NUL, B0_HNAME_SIZE) + && memchr(b0p->b0_fname, NUL, B0_FNAME_SIZE_CRYPT)); +} + /* * Update the timestamp or the B0_SAME_DIR flag of the .swp file. */ @@ -1522,6 +1532,8 @@ static time_t swapfile_info(char_u *fname) MSG_PUTS(_(" [from Vim version 3.0]")); } else if (ml_check_b0_id(&b0) == FAIL) { MSG_PUTS(_(" [does not look like a Vim swap file]")); + } else if (!ml_check_b0_strings(&b0)) { + MSG_PUTS(_(" [garbled strings (not nul terminated)]")); } else { MSG_PUTS(_(" file name: ")); if (b0.b0_fname[0] == NUL) diff --git a/src/nvim/vim.h b/src/nvim/vim.h index 29d61dcbde..410c2602c8 100644 --- a/src/nvim/vim.h +++ b/src/nvim/vim.h @@ -322,13 +322,10 @@ enum { (size_t)(n)) #ifndef EINTR -# define read_eintr(fd, buf, count) vim_read((fd), (buf), (count)) -# define write_eintr(fd, buf, count) vim_write((fd), (buf), (count)) +# define read_eintr(fd, buf, count) read((fd), (buf), (count)) +# define write_eintr(fd, buf, count) write((fd), (buf), (count)) #endif -# define vim_read(fd, buf, count) read((fd), (char *)(buf), (size_t) (count)) -# define vim_write(fd, buf, count) write((fd), (char *)(buf), (size_t) (count)) - /* * Enums need a typecast to be used as array index (for Ultrix). */ -- cgit From 3db0a40d691c103a26ef3df74528f12d89b0fa61 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Fri, 20 Feb 2015 17:32:42 +0100 Subject: coverity/105568: Free of array-typed value: FP. Problem : Free of array-typed value @ 3628. Diagnostic : False positive. Rationale : expand_shell_cmd() is called with a mock value for file (*file = (char_u **)""). That means we want file to be filled with a new value. We can't use *file = NULL because that means we don't want file to be filled. Now, coverity incorrectly thinks that sentinel value is the one we are freeing up at some other later point, which is not the case. Resolution : Assert that, when we are freeing *file, its value is different than the sentinel one. --- src/nvim/ex_getln.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src') diff --git a/src/nvim/ex_getln.c b/src/nvim/ex_getln.c index d5f7a218f4..238beebf3e 100644 --- a/src/nvim/ex_getln.c +++ b/src/nvim/ex_getln.c @@ -10,6 +10,7 @@ * ex_getln.c: Functions for entering and editing an Ex command line. */ +#include #include #include #include @@ -3858,8 +3859,10 @@ expand_shellcmd ( STRLCPY(buf + l, pat, MAXPATHL - l); /* Expand matches in one directory of $PATH. */ + char_u **prev_file = *file; ret = expand_wildcards(1, &buf, num_file, file, flags); if (ret == OK) { + assert(*file != prev_file); ga_grow(&ga, *num_file); { for (i = 0; i < *num_file; ++i) { -- cgit From a2b4535747afcbd374cd97bbfd47457c52319723 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Tue, 17 Mar 2015 18:45:26 +0100 Subject: coverity/105982: Unckecked return value: RI. Problem : Unchecked return value from library @ 91. Diagnostic : Real issue. Rationale : fcntl can fail, which is not being checked. Resolution : Add corresponding error handling. Helped-by: oni-link --- src/nvim/os/pty_process.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/nvim/os/pty_process.c b/src/nvim/os/pty_process.c index bd7247c741..a1edebb846 100644 --- a/src/nvim/os/pty_process.c +++ b/src/nvim/os/pty_process.c @@ -40,6 +40,7 @@ typedef struct { void pty_process_init(Job *job) { PtyProcess *ptyproc = xmalloc(sizeof(PtyProcess)); + ptyproc->tty_fd = -1; if (job->opts.writable) { uv_pipe_init(uv_default_loop(), &ptyproc->proc_stdin, 0); @@ -69,6 +70,9 @@ void pty_process_destroy(Job *job) job->process = NULL; } +static const unsigned int KILL_RETRIES = 5; +static const unsigned int KILL_TIMEOUT = 2; // seconds + bool pty_process_spawn(Job *job) { int master; @@ -88,7 +92,15 @@ bool pty_process_spawn(Job *job) } // make sure the master file descriptor is non blocking - fcntl(master, F_SETFL, fcntl(master, F_GETFL) | O_NONBLOCK); + int master_status_flags = fcntl(master, F_GETFL); + if (master_status_flags == -1) { + ELOG("Failed to get master descriptor status flags: %s", strerror(errno)); + goto error; + } + if (fcntl(master, F_SETFL, master_status_flags | O_NONBLOCK) == -1) { + ELOG("Failed to make master descriptor non-blocking: %s", strerror(errno)); + goto error; + } if (job->opts.writable) { uv_pipe_open(&ptyproc->proc_stdin, dup(master)); @@ -108,6 +120,22 @@ bool pty_process_spawn(Job *job) ptyproc->tty_fd = master; job->pid = pid; return true; + +error: + close(master); + + // terminate spawned process + kill(pid, SIGTERM); + int status, child; + unsigned int try = 0; + while (try++ < KILL_RETRIES && !(child = waitpid(pid, &status, WNOHANG))) { + sleep(KILL_TIMEOUT); + } + if (child != pid) { + kill(pid, SIGKILL); + } + + return false; } void pty_process_close(Job *job) @@ -115,10 +143,20 @@ void pty_process_close(Job *job) PtyProcess *ptyproc = job->process; uv_signal_stop(&ptyproc->schld); uv_close((uv_handle_t *)&ptyproc->schld, NULL); + pty_process_close_master(job); job_close_streams(job); job_decref(job); } +void pty_process_close_master(Job *job) +{ + PtyProcess *ptyproc = job->process; + if (ptyproc->tty_fd >= 0) { + close(ptyproc->tty_fd); + ptyproc->tty_fd = -1; + } +} + void pty_process_resize(Job *job, uint16_t width, uint16_t height) { PtyProcess *ptyproc = job->process; -- cgit From c6784d9f6f2bcb648072486d90390e3760c579c6 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Tue, 17 Mar 2015 19:08:44 +0100 Subject: coverity/105985: Resource leak: RI. Problem : Resource leak @ 94, 98, 102. Diagnostic : Real issue. Rationale : Coverity doesn't know that uv_pipe_open will save file descriptor to close them later. So, it signals file descriptors being leaked. This would then seem like a false positive we can fix by teaching coverity about uv_pipe_open through model file. But then we realize that the above is only true if uv_pipe_open succeeds. It it fails, then descriptors are really being leaked, which is why this is considered a real issue and not a false positive after all. Resolution : Add error handling to correctly close descriptors if uv_pipe_open fails at any point. Add model for uv_pipe_open so that Coverity knows it will save descriptors when no error. Helped-by: oni-link --- src/nvim/os/pty_process.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/nvim/os/pty_process.c b/src/nvim/os/pty_process.c index a1edebb846..ed98d3e0c0 100644 --- a/src/nvim/os/pty_process.c +++ b/src/nvim/os/pty_process.c @@ -70,6 +70,23 @@ void pty_process_destroy(Job *job) job->process = NULL; } +static bool set_pipe_duplicating_descriptor(int fd, uv_pipe_t *pipe) +{ + int fd_dup = dup(fd); + if (fd_dup < 0) { + ELOG("Failed to dup descriptor %d: %s", fd, strerror(errno)); + return false; + } + int uv_result = uv_pipe_open(pipe, fd_dup); + if (uv_result) { + ELOG("Failed to set pipe to descriptor %d: %s", + fd_dup, uv_strerror(uv_result)); + close(fd_dup); + return false; + } + return true; +} + static const unsigned int KILL_RETRIES = 5; static const unsigned int KILL_TIMEOUT = 2; // seconds @@ -102,16 +119,19 @@ bool pty_process_spawn(Job *job) goto error; } - if (job->opts.writable) { - uv_pipe_open(&ptyproc->proc_stdin, dup(master)); + if (job->opts.writable + && !set_pipe_duplicating_descriptor(master, &ptyproc->proc_stdin)) { + goto error; } - if (job->opts.stdout_cb) { - uv_pipe_open(&ptyproc->proc_stdout, dup(master)); + if (job->opts.stdout_cb + && !set_pipe_duplicating_descriptor(master, &ptyproc->proc_stdout)) { + goto error; } - if (job->opts.stderr_cb) { - uv_pipe_open(&ptyproc->proc_stderr, dup(master)); + if (job->opts.stderr_cb + && !set_pipe_duplicating_descriptor(master, &ptyproc->proc_stderr)) { + goto error; } uv_signal_init(uv_default_loop(), &ptyproc->schld); -- cgit From 44b563409e8c67d4116ff2e2b726cda4e4dc03f6 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Sun, 22 Mar 2015 10:51:13 +0100 Subject: Passing-by: Add function attributes. --- src/nvim/os/pty_process.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/nvim/os/pty_process.c b/src/nvim/os/pty_process.c index ed98d3e0c0..c135efc6d3 100644 --- a/src/nvim/os/pty_process.c +++ b/src/nvim/os/pty_process.c @@ -20,6 +20,7 @@ #include +#include "nvim/func_attr.h" #include "nvim/os/job.h" #include "nvim/os/job_defs.h" #include "nvim/os/job_private.h" @@ -37,7 +38,7 @@ typedef struct { int tty_fd; } PtyProcess; -void pty_process_init(Job *job) +void pty_process_init(Job *job) FUNC_ATTR_NONNULL_ALL { PtyProcess *ptyproc = xmalloc(sizeof(PtyProcess)); ptyproc->tty_fd = -1; @@ -63,7 +64,7 @@ void pty_process_init(Job *job) job->process = ptyproc; } -void pty_process_destroy(Job *job) +void pty_process_destroy(Job *job) FUNC_ATTR_NONNULL_ALL { free(job->opts.term_name); free(job->process); @@ -71,6 +72,7 @@ void pty_process_destroy(Job *job) } static bool set_pipe_duplicating_descriptor(int fd, uv_pipe_t *pipe) + FUNC_ATTR_NONNULL_ALL { int fd_dup = dup(fd); if (fd_dup < 0) { @@ -90,7 +92,7 @@ static bool set_pipe_duplicating_descriptor(int fd, uv_pipe_t *pipe) static const unsigned int KILL_RETRIES = 5; static const unsigned int KILL_TIMEOUT = 2; // seconds -bool pty_process_spawn(Job *job) +bool pty_process_spawn(Job *job) FUNC_ATTR_NONNULL_ALL { int master; PtyProcess *ptyproc = job->process; @@ -158,7 +160,7 @@ error: return false; } -void pty_process_close(Job *job) +void pty_process_close(Job *job) FUNC_ATTR_NONNULL_ALL { PtyProcess *ptyproc = job->process; uv_signal_stop(&ptyproc->schld); @@ -168,7 +170,7 @@ void pty_process_close(Job *job) job_decref(job); } -void pty_process_close_master(Job *job) +void pty_process_close_master(Job *job) FUNC_ATTR_NONNULL_ALL { PtyProcess *ptyproc = job->process; if (ptyproc->tty_fd >= 0) { @@ -178,13 +180,14 @@ void pty_process_close_master(Job *job) } void pty_process_resize(Job *job, uint16_t width, uint16_t height) + FUNC_ATTR_NONNULL_ALL { PtyProcess *ptyproc = job->process; ptyproc->winsize = (struct winsize){height, width, 0, 0}; ioctl(ptyproc->tty_fd, TIOCSWINSZ, &ptyproc->winsize); } -static void init_child(Job *job) +static void init_child(Job *job) FUNC_ATTR_NONNULL_ALL { unsetenv("COLUMNS"); unsetenv("LINES"); @@ -204,7 +207,7 @@ static void init_child(Job *job) fprintf(stderr, "execvp failed: %s\n", strerror(errno)); } -static void chld_handler(uv_signal_t *handle, int signum) +static void chld_handler(uv_signal_t *handle, int signum) FUNC_ATTR_NONNULL_ALL { Job *job = handle->data; int stat = 0; @@ -229,7 +232,7 @@ static void chld_handler(uv_signal_t *handle, int signum) pty_process_close(job); } -static void init_termios(struct termios *termios) +static void init_termios(struct termios *termios) FUNC_ATTR_NONNULL_ALL { memset(termios, 0, sizeof(struct termios)); // Taken from pangoterm -- cgit