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)!
This commit is contained in:
James McCoy 2017-08-08 22:18:50 +02:00 committed by Justin M. Keyes
parent 085102fadf
commit ac055d677a
2 changed files with 10 additions and 4 deletions

View File

@ -605,8 +605,11 @@ int os_fsync(int fd)
/// ///
/// @return libuv return code. /// @return libuv return code.
static int os_stat(const char *name, uv_stat_t *statbuf) 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; uv_fs_t request;
int result = uv_fs_stat(&fs_loop, &request, name, NULL); int result = uv_fs_stat(&fs_loop, &request, name, NULL);
*statbuf = request.statbuf; *statbuf = request.statbuf;
@ -618,7 +621,6 @@ static int os_stat(const char *name, uv_stat_t *statbuf)
/// ///
/// @return libuv error code on error. /// @return libuv error code on error.
int32_t os_getperm(const char *name) int32_t os_getperm(const char *name)
FUNC_ATTR_NONNULL_ALL
{ {
uv_stat_t statbuf; uv_stat_t statbuf;
int stat_result = os_stat(name, &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 /// @return `true` if `path` exists
bool os_path_exists(const char_u *path) bool os_path_exists(const char_u *path)
FUNC_ATTR_NONNULL_ALL
{ {
uv_stat_t statbuf; uv_stat_t statbuf;
return os_stat((char *)path, &statbuf) == kLibuvSuccess; 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. /// @param[out] file_info Pointer to a FileInfo to put the information in.
/// @return `true` on success, `false` for failure. /// @return `true` on success, `false` for failure.
bool os_fileinfo(const char *path, FileInfo *file_info) 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; return os_stat(path, &(file_info->stat)) == kLibuvSuccess;
} }

View File

@ -862,6 +862,11 @@ describe('fs.c', function()
end end
describe('os_fileinfo', function() 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() itp('returns false if given a non-existing file', function()
local file_info = file_info_new() local file_info = file_info_new()
assert.is_false((fs.os_fileinfo('/non-existent', file_info))) assert.is_false((fs.os_fileinfo('/non-existent', file_info)))