From 17eb20b22ef9fe5e621a4b6eff1c1a83e9489645 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Wed, 9 Aug 2017 13:30:34 -0400 Subject: log: Add log_callstack_to_file() This makes it trivial to log the callstack to, e.g., stderr, which can simplify debug cycles. --- src/nvim/README.md | 5 +++-- src/nvim/log.c | 25 ++++++++++++++++--------- src/nvim/log.h | 1 + 3 files changed, 20 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/nvim/README.md b/src/nvim/README.md index 1c1c3c364e..0caf71e2c5 100644 --- a/src/nvim/README.md +++ b/src/nvim/README.md @@ -11,8 +11,9 @@ Logs Low-level log messages sink to `$NVIM_LOG_FILE`. -You can use `LOG_CALLSTACK()` anywhere in the source to log the current -stacktrace. (Currently Linux-only.) +You can use `LOG_CALLSTACK();` anywhere in the source to log the current +stacktrace. To log in an alternate file, e.g. stderr, use +`LOG_CALLSTACK_TO_FILE(FILE*)`. (Currently Linux-only.) UI events are logged at level 0 (`DEBUG_LOG_LEVEL`). diff --git a/src/nvim/log.c b/src/nvim/log.c index b64aef3cac..3baf0b2ebd 100644 --- a/src/nvim/log.c +++ b/src/nvim/log.c @@ -178,7 +178,8 @@ FILE *open_log_file(void) } #ifdef HAVE_EXECINFO_BACKTRACE -void log_callstack(const char *const func_name, const int line_num) +void log_callstack_to_file(FILE *log_file, const char *const func_name, + const int line_num) { void *trace[100]; int trace_size = backtrace(trace, ARRAY_SIZE(trace)); @@ -190,8 +191,6 @@ void log_callstack(const char *const func_name, const int line_num) } assert(24 + exepathlen < IOSIZE); // Must fit in `cmdbuf` below. - do_log(DEBUG_LOG_LEVEL, func_name, line_num, true, "trace:"); - char cmdbuf[IOSIZE + (20 * ARRAY_SIZE(trace))]; snprintf(cmdbuf, sizeof(cmdbuf), "addr2line -e %s -f -p", exepath); for (int i = 1; i < trace_size; i++) { @@ -202,12 +201,8 @@ void log_callstack(const char *const func_name, const int line_num) // Now we have a command string like: // addr2line -e /path/to/exe -f -p 0x123 0x456 ... - log_lock(); - FILE *log_file = open_log_file(); - if (log_file == NULL) { - goto end; - } - + do_log_to_file(log_file, DEBUG_LOG_LEVEL, func_name, line_num, true, + "trace:"); FILE *fp = popen(cmdbuf, "r"); char linebuf[IOSIZE]; while (fgets(linebuf, sizeof(linebuf) - 1, fp) != NULL) { @@ -218,6 +213,18 @@ void log_callstack(const char *const func_name, const int line_num) if (log_file != stderr && log_file != stdout) { fclose(log_file); } +} + +void log_callstack(const char *const func_name, const int line_num) +{ + log_lock(); + FILE *log_file = open_log_file(); + if (log_file == NULL) { + goto end; + } + + log_callstack_to_file(log_file, func_name, line_num); + end: log_unlock(); } diff --git a/src/nvim/log.h b/src/nvim/log.h index 743a8d17aa..5064d9333b 100644 --- a/src/nvim/log.h +++ b/src/nvim/log.h @@ -63,6 +63,7 @@ #ifdef HAVE_EXECINFO_BACKTRACE # define LOG_CALLSTACK() log_callstack(__func__, __LINE__) +# define LOG_CALLSTACK_TO_FILE(fp) log_callstack_to_file(fp, __func__, __LINE__) #endif #ifdef INCLUDE_GENERATED_DECLARATIONS -- cgit From ac055d677aa9eff9fca11cecb5ac7f7a4edb0265 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Tue, 8 Aug 2017 22:18:50 +0200 Subject: os_stat: return ENOENT on NULL filename arg Closes #4370 Explication: In the backtrace in #4370, we see that `buf_write()` was called with non-NULL `fname` and `sfname` arguments, but they've since _become_ NULL. #7 0x00000000004de09d in buf_write (buf=0x1dee040, fname=0x0, fname@entry=0x1e985b0 "/home/sean/src/github.com/snczl/virta/pkg/meld/segment.go", sfname=0x0, sfname@entry=0x1ddfa60 "segment.go", start=1, end=72, eap=eap@entry=0x7ffc6b032e60, append=0, forceit=0, reset_changed=1, filtering=0) at /home/travis/build/neovim/bot-ci/build/neovim/src/nvim/fileio.c:2576 This is most likely due to the code that restores those values from `buf`, which happens just before the fatal call to `os_fileinfo` ```c /* * The autocommands may have changed the name of the buffer, which may * be kept in fname, ffname and sfname. */ if (buf_ffname) ffname = buf->b_ffname; if (buf_sfname) sfname = buf->b_sfname; if (buf_fname_f) fname = buf->b_ffname; if (buf_fname_s) fname = buf->b_sfname; ``` It's worth noting that at this point `ffname` is still non-NULL, so it _could_ be used. However, our current code is purely more strict than Vim in this area, which has caused us problems before (e.g., `getdigits()`). The commentary for `struct file_buffer` clearly indicate that all of `b_ffname`, `b_sfname`, and `b_fname` may be NULL: ```c /* * b_ffname has the full path of the file (NULL for no name). * b_sfname is the name as the user typed it (or NULL). * b_fname is the same as b_sfname, unless ":cd" has been done, * then it is the same as b_ffname (NULL for no name). */ char_u *b_ffname; /* full path file name */ char_u *b_sfname; /* short file name */ char_u *b_fname; /* current file name */ ``` Vim directly calls `stat(2)` which, although it is annotated to tell the compiler that the path argument is non-NULL, does handle a NULL pointer by returning a `-1` value and setting `errno` to `EFAULT`. This satisfies Vim's check, since it treats any `-1` return from `stat(2)` to mean the file doesn't exist (at least in this code path). Note that Vim's mch_stat() implementations on win32 and solaris clearly cannot accept NULL `name`. But the codepaths that call mch_stat will NULL `name` tend to be unix-only (eg: u_read_undo)! --- src/nvim/os/fs.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c index 9e3025cf89..6ac9d682d7 100644 --- a/src/nvim/os/fs.c +++ b/src/nvim/os/fs.c @@ -605,8 +605,11 @@ int os_fsync(int fd) /// /// @return libuv return code. static int os_stat(const char *name, uv_stat_t *statbuf) - FUNC_ATTR_NONNULL_ALL + FUNC_ATTR_NONNULL_ARG(2) { + if (!name) { + return UV_ENOENT; + } uv_fs_t request; int result = uv_fs_stat(&fs_loop, &request, name, NULL); *statbuf = request.statbuf; @@ -618,7 +621,6 @@ static int os_stat(const char *name, uv_stat_t *statbuf) /// /// @return libuv error code on error. int32_t os_getperm(const char *name) - FUNC_ATTR_NONNULL_ALL { uv_stat_t statbuf; int stat_result = os_stat(name, &statbuf); @@ -657,7 +659,6 @@ int os_fchown(int fd, uv_uid_t owner, uv_gid_t group) /// /// @return `true` if `path` exists bool os_path_exists(const char_u *path) - FUNC_ATTR_NONNULL_ALL { uv_stat_t statbuf; return os_stat((char *)path, &statbuf) == kLibuvSuccess; @@ -847,7 +848,7 @@ int os_remove(const char *path) /// @param[out] file_info Pointer to a FileInfo to put the information in. /// @return `true` on success, `false` for failure. bool os_fileinfo(const char *path, FileInfo *file_info) - FUNC_ATTR_NONNULL_ALL + FUNC_ATTR_NONNULL_ARG(2) { return os_stat(path, &(file_info->stat)) == kLibuvSuccess; } -- cgit From 7ae744b93d80fd169507732931606276844150b6 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Thu, 10 Aug 2017 01:40:21 +0200 Subject: buf_write(): handle NULL fname on non-unix --- src/nvim/fileio.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c index 643020df5e..feb16f44d4 100644 --- a/src/nvim/fileio.c +++ b/src/nvim/fileio.c @@ -2570,11 +2570,9 @@ buf_write ( perm = -1; } } -#else /* win32 */ - /* - * Check for a writable device name. - */ - c = os_nodetype((char *)fname); +#else // win32 + // Check for a writable device name. + c = fname == NULL ? NODE_OTHER : os_nodetype((char *)fname); if (c == NODE_OTHER) { SET_ERRMSG_NUM("E503", _("is not a file or writable device")); goto fail; @@ -2594,9 +2592,8 @@ buf_write ( if (overwriting) { os_fileinfo((char *)fname, &file_info_old); } - } -#endif /* !UNIX */ +#endif // !UNIX if (!device && !newfile) { /* -- cgit From b7e84de7d2385670aadcea9c02bc68206db98862 Mon Sep 17 00:00:00 2001 From: KunMing Xie Date: Thu, 10 Aug 2017 10:20:55 +0800 Subject: vim-patch:8.0.0165 (#7132) Problem: Ubsan warns for integer overflow. Solution: Swap two conditions. (Dominique Pelle) https://github.com/vim/vim/commit/f446b48ff0bffae2b453cd4f9e3c25dfe363d29d --- src/nvim/regexp_nfa.c | 16 ++++++++-------- src/nvim/version.c | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/nvim/regexp_nfa.c b/src/nvim/regexp_nfa.c index 5d708febea..93ba9ce097 100644 --- a/src/nvim/regexp_nfa.c +++ b/src/nvim/regexp_nfa.c @@ -56,13 +56,13 @@ enum { NFA_RANGE_MIN, /* low end of a range */ NFA_RANGE_MAX, /* high end of a range */ - NFA_CONCAT, /* concatenate two previous items (postfix - * only) */ - NFA_OR, /* \| (postfix only) */ - NFA_STAR, /* greedy * (posfix only) */ - NFA_STAR_NONGREEDY, /* non-greedy * (postfix only) */ - NFA_QUEST, /* greedy \? (postfix only) */ - NFA_QUEST_NONGREEDY, /* non-greedy \? (postfix only) */ + NFA_CONCAT, // concatenate two previous items (postfix + // only) + NFA_OR, // \| (postfix only) + NFA_STAR, // greedy * (postfix only) + NFA_STAR_NONGREEDY, // non-greedy * (postfix only) + NFA_QUEST, // greedy \? (postfix only) + NFA_QUEST_NONGREEDY, // non-greedy \? (postfix only) NFA_BOL, /* ^ Begin line */ NFA_EOL, /* $ End line */ @@ -1988,7 +1988,7 @@ static int nfa_regpiece(void) // The engine is very inefficient (uses too many states) when the maximum // is much larger than the minimum and when the maximum is large. Bail out // if we can use the other engine. - if ((nfa_re_flags & RE_AUTO) && (maxval > minval + 200 || maxval > 500)) { + if ((nfa_re_flags & RE_AUTO) && (maxval > 500 || maxval > minval + 200)) { return FAIL; } diff --git a/src/nvim/version.c b/src/nvim/version.c index fa179b4f63..72af7dbafd 100644 --- a/src/nvim/version.c +++ b/src/nvim/version.c @@ -787,7 +787,7 @@ static const int included_patches[] = { 168, 167, // 166, - // 165, + 165, // 164, // 163 NA // 162 NA -- cgit From 9edf00bddf6802044e06bd15daad4a5735ccade8 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Fri, 11 Aug 2017 10:30:38 -0400 Subject: coverity/166184: Check length of str, not term 32396b5879b429def1c48948069c55366d41b9be add length checks to TERMINAL_FAMILY/STARTS_WITH to ensure memcmp() wouldn't read past the end of the string. However, "term" was copy/pasted from TERMINAL_FAMILY so STARTS_WITH() was unnecessarily reading the, potentially NULL, term variable. --- src/nvim/tui/tui.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c index c29ec09638..97a0398c80 100644 --- a/src/nvim/tui/tui.c +++ b/src/nvim/tui/tui.c @@ -45,7 +45,7 @@ #define OUTBUF_SIZE 0xffff #define TOO_MANY_EVENTS 1000000 -#define STARTS_WITH(str, prefix) (strlen(term) >= (sizeof(prefix) - 1) \ +#define STARTS_WITH(str, prefix) (strlen(str) >= (sizeof(prefix) - 1) \ && 0 == memcmp((str), (prefix), sizeof(prefix) - 1)) #define TMUX_WRAP(is_tmux, seq) ((is_tmux) \ ? "\x1bPtmux;\x1b" seq "\x1b\\" : seq) -- cgit