From ac055d677aa9eff9fca11cecb5ac7f7a4edb0265 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Tue, 8 Aug 2017 22:18:50 +0200 Subject: [PATCH 1/2] 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 +++++---- test/unit/os/fs_spec.lua | 5 +++++ 2 files changed, 10 insertions(+), 4 deletions(-) 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; } diff --git a/test/unit/os/fs_spec.lua b/test/unit/os/fs_spec.lua index 23eb05b4b8..bc39507c08 100644 --- a/test/unit/os/fs_spec.lua +++ b/test/unit/os/fs_spec.lua @@ -862,6 +862,11 @@ describe('fs.c', function() end describe('os_fileinfo', function() + itp('returns false if path=NULL', function() + local file_info = file_info_new() + assert.is_false((fs.os_fileinfo(nil, file_info))) + end) + itp('returns false if given a non-existing file', function() local file_info = file_info_new() assert.is_false((fs.os_fileinfo('/non-existent', file_info))) From 7ae744b93d80fd169507732931606276844150b6 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Thu, 10 Aug 2017 01:40:21 +0200 Subject: [PATCH 2/2] buf_write(): handle NULL fname on non-unix --- src/nvim/fileio.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) 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) { /*