From f644d8d88e621ab11dcb831b3cee6c5ea1df24b2 Mon Sep 17 00:00:00 2001 From: HiPhish Date: Tue, 26 Apr 2016 18:10:36 +0200 Subject: Fix coverity errors in haslocaldir() and getcwd. The Vim function `haslocaldir()` would crash if the users called it with the two arguments `-1, -1`. Now it returns `0` in that case. The coverity issue was complaining about a NULL dereference, but there can never be a case where the pointer `tp` is NULL and being dereferenced. An assertion has been put in place to satisfy coverity. Furthermore the functions themselves have been cleaned up. First of all the documentation comment for the different scopes has been extended and a macro for the minimum scope has been introduced. In both functions any time a scope is used as a range (e.g. in a loop) macros instead of actuals scopes are used, that makes the functions more robust if new scopes are added. Second, in the implementation of `getcwd()` there was a superfluous loop, it has been removed completely. I also changed all `goto end` to plaing `return` statements by moving the allocation of `cwd` down, that way there is no need for `goto` anymore. --- src/nvim/eval.c | 58 +++++++++++++++++++++++------------------------------ src/nvim/ex_docmd.h | 12 ++++++----- 2 files changed, 32 insertions(+), 38 deletions(-) (limited to 'src') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index 0ff70df54d..5323e17158 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -9820,7 +9820,7 @@ static void f_getcmdwintype(typval_T *argvars, typval_T *rettv) static void f_getcwd(typval_T *argvars, typval_T *rettv) { // Possible scope of working directory to return. - CdScope scope = kCdScopeWindow; + CdScope scope = MIN_CD_SCOPE; // Numbers of the scope objects (window, tab) we want the working directory // of. A `-1` means to skip this scope, a `0` means the current object. @@ -9839,7 +9839,8 @@ static void f_getcwd(typval_T *argvars, typval_T *rettv) rettv->vval.v_string = NULL; // Pre-conditions and scope extraction together - for (int i = 0; i < kCdScopeGlobal; i++) { + for (int i = MIN_CD_SCOPE; i < MAX_CD_SCOPE; i++) { + // If there is no argument there are no more scopes after it, break out. if (argvars[i].v_type == VAR_UNKNOWN) { break; } @@ -9850,34 +9851,17 @@ static void f_getcwd(typval_T *argvars, typval_T *rettv) scope_number[i] = argvars[i].vval.v_number; // The scope is the current iteration step. scope = i; - - if (scope_number[i] < -1) { - EMSG(_(e_invarg)); - return; - } - } - - // Allocate and initialize the string to return. - cwd = xmalloc(MAXPATHL); - - // Get the scope and numbers from the arguments - for (int i = 0; i < MAX_CD_SCOPE; i++) { - // If there is no argument there are no more scopes after it, break out. - if (argvars[i].v_type == VAR_UNKNOWN) { - break; - } - scope_number[i] = argvars[i].vval.v_number; - // The scope is the current iteration step. - scope = i; // It is an error for the scope number to be less than `-1`. if (scope_number[i] < -1) { EMSG(_(e_invarg)); - goto end; + return; } } - // If the deepest scope number is `-1` advance the scope. + // Normalize scope, the number of the new scope will be 0. if (scope_number[scope] < 0) { + // Arguments to `getcwd` always end at second-highest scope, so scope will + // always be <= `MAX_CD_SCOPE`. scope++; } @@ -9888,7 +9872,7 @@ static void f_getcwd(typval_T *argvars, typval_T *rettv) tp = find_tabpage(scope_number[kCdScopeTab]); if (!tp) { EMSG(_("E5000: Cannot find tab number.")); - goto end; + return; } } @@ -9898,25 +9882,29 @@ static void f_getcwd(typval_T *argvars, typval_T *rettv) } else if (scope_number[kCdScopeWindow] >= 0) { if (!tp) { EMSG(_("E5001: Higher scope cannot be -1 if lower scope is >= 0.")); - goto end; + return; } if (scope_number[kCdScopeWindow] > 0) { win = find_win_by_nr(&argvars[0], curtab); if (!win) { EMSG(_("E5002: Cannot find window number.")); - goto end; + return; } } } + cwd = xmalloc(MAXPATHL); + switch (scope) { case kCdScopeWindow: + assert(win); from = win->w_localdir; if (from) { break; } case kCdScopeTab: // FALLTHROUGH + assert(tp); from = tp->localdir; if (from) { break; @@ -9926,7 +9914,7 @@ static void f_getcwd(typval_T *argvars, typval_T *rettv) if (globaldir) { from = globaldir; } else { - // Copy the OS path directly into output string and jump to the end. + // We have to copy the OS path directly into output string. if (os_dirname(cwd, MAXPATHL) == FAIL) { EMSG(_("E41: Could not display path.")); goto end; @@ -9943,7 +9931,6 @@ static void f_getcwd(typval_T *argvars, typval_T *rettv) #ifdef BACKSLASH_IN_FILENAME slash_adjust(rettv->vval.v_string); #endif - end: xfree(cwd); } @@ -10779,7 +10766,7 @@ static void f_has_key(typval_T *argvars, typval_T *rettv) static void f_haslocaldir(typval_T *argvars, typval_T *rettv) { // Possible scope of working directory to return. - CdScope scope = kCdScopeWindow; + CdScope scope = MIN_CD_SCOPE; // Numbers of the scope objects (window, tab) we want the working directory // of. A `-1` means to skip this scope, a `0` means the current object. @@ -10795,7 +10782,7 @@ static void f_haslocaldir(typval_T *argvars, typval_T *rettv) rettv->vval.v_number = 0; // Pre-conditions and scope extraction together - for (int i = 0; i < kCdScopeGlobal; i++) { + for (int i = MIN_CD_SCOPE; i < MAX_CD_SCOPE; i++) { if (argvars[i].v_type == VAR_UNKNOWN) { break; } @@ -10812,9 +10799,11 @@ static void f_haslocaldir(typval_T *argvars, typval_T *rettv) } } - // It the deepest scope number is `-1` advance the scope by one. + // Normalize scope, the number of the new scope will be 0. if (scope_number[scope] < 0) { - ++scope; + // Arguments to `haslocaldir` always end at second-highest scope, so scope + // will always be <= `MAX_CD_SCOPE`. + scope++; } // Find the tabpage by number @@ -10848,13 +10837,16 @@ static void f_haslocaldir(typval_T *argvars, typval_T *rettv) switch (scope) { case kCdScopeWindow: + assert(win); rettv->vval.v_number = win->w_localdir ? 1 : 0; break; case kCdScopeTab: + assert(tp); rettv->vval.v_number = tp->localdir ? 1 : 0; break; case kCdScopeGlobal: - assert(0); + // The global scope never has a local directory + rettv->vval.v_number = 0; break; } } diff --git a/src/nvim/ex_docmd.h b/src/nvim/ex_docmd.h index 7af3ee233c..dbfc64e2f1 100644 --- a/src/nvim/ex_docmd.h +++ b/src/nvim/ex_docmd.h @@ -19,16 +19,18 @@ #define EXMODE_NORMAL 1 #define EXMODE_VIM 2 -/// The scope of a command. +/// The scope of a working-directory command like `:cd`. /// -/// The lower a number, the deeper the scope. +/// Scopes are enumerated from lowest to highest. When adding a scope make sure +/// to update all functions using scopes as well, such as the implementation of +/// `getcwd()`. When using scopes as limits (e.g. in loops) don't use the scopes +/// directly, use `MIN_CD_SCOPE` and `MAX_CD_SCOPE` instead. typedef enum { kCdScopeWindow, ///< Affects one window. kCdScopeTab, ///< Affects one tab page. - kCdScopeGlobal, ///< Affects the entire instance of NeoVim. + kCdScopeGlobal, ///< Affects the entire instance of Neovim. } CdScope; - -/// Last `:cd` scope defined. +#define MIN_CD_SCOPE kCdScopeWindow #define MAX_CD_SCOPE kCdScopeGlobal #ifdef INCLUDE_GENERATED_DECLARATIONS -- cgit