From 8d982ab52269e8adccbc21cc0d6f8ab3b817bf6e Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 20:55:48 +0300 Subject: [PATCH 1/9] coverity/13686: Do not allow NUL byte in precondition regex Before this commit it emitted e_spell_trunc in the first case and treated file as completely valid on the second. While first is fine (both errors are actually valid, though old error is probably better), second results in incorrect regex used. --- src/nvim/spellfile.c | 41 ++++++++-------- test/functional/spell/spellfile_spec.lua | 61 ++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 20 deletions(-) create mode 100644 test/functional/spell/spellfile_spec.lua diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index bbef1f5032..2c0db0694a 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -267,7 +267,7 @@ #define SAL_REM_ACCENTS 4 #define VIMSPELLMAGIC "VIMspell" // string at start of Vim spell file -#define VIMSPELLMAGICL 8 +#define VIMSPELLMAGICL (sizeof(VIMSPELLMAGIC) - 1) #define VIMSPELLVERSION 50 // Section IDs. Only renumber them when VIMSPELLVERSION changes! @@ -516,7 +516,6 @@ spell_load_file ( FILE *fd; char_u buf[VIMSPELLMAGICL]; char_u *p; - int i; int n; int len; char_u *save_sourcing_name = sourcing_name; @@ -558,8 +557,9 @@ spell_load_file ( sourcing_lnum = 0; //
: - for (i = 0; i < VIMSPELLMAGICL; ++i) + for (size_t i = 0; i < VIMSPELLMAGICL; i++) { buf[i] = getc(fd); // + } if (STRNCMP(buf, VIMSPELLMAGIC, VIMSPELLMAGICL) != 0) { EMSG(_("E757: This does not look like a spell file")); goto endFAIL; @@ -983,35 +983,36 @@ static int read_charflags_section(FILE *fd) // Return SP_*ERROR flags. static int read_prefcond_section(FILE *fd, slang_T *lp) { - int cnt; - int i; - int n; - char_u *p; - char_u buf[MAXWLEN + 1]; - // ... - cnt = get2c(fd); // - if (cnt <= 0) + const int cnt = get2c(fd); // + if (cnt <= 0) { return SP_FORMERROR; + } lp->sl_prefprog = xcalloc(cnt, sizeof(regprog_T *)); lp->sl_prefixcnt = cnt; - for (i = 0; i < cnt; ++i) { + for (int i = 0; i < cnt; ++i) { // : - n = getc(fd); // - if (n < 0 || n >= MAXWLEN) + const int n = getc(fd); // + if (n < 0 || n >= MAXWLEN) { return SP_FORMERROR; + } // When is zero we have an empty condition. Otherwise // compile the regexp program used to check for the condition. if (n > 0) { - buf[0] = '^'; // always match at one position only - p = buf + 1; - while (n-- > 0) - *p++ = getc(fd); // - *p = NUL; - lp->sl_prefprog[i] = vim_regcomp(buf, RE_MAGIC + RE_STRING); + char buf[MAXWLEN + 1]; + buf[0] = '^'; // always match at one position only + const size_t read_byte = fread(buf + 1, 1, (size_t)n, fd); + if (read_byte != (size_t)n) { + return feof(fd) ? SP_FORMERROR : SP_OTHERERROR; + } + if (memchr(buf + 1, NUL, (size_t)n)) { + return SP_FORMERROR; + } + buf[n + 1] = NUL; + lp->sl_prefprog[i] = vim_regcomp((char_u *)buf, RE_MAGIC | RE_STRING); } } return 0; diff --git a/test/functional/spell/spellfile_spec.lua b/test/functional/spell/spellfile_spec.lua new file mode 100644 index 0000000000..88f757249e --- /dev/null +++ b/test/functional/spell/spellfile_spec.lua @@ -0,0 +1,61 @@ +local helpers = require('test.functional.helpers')(after_each) +local lfs = require('lfs') + +local eq = helpers.eq +local clear = helpers.clear +local meths = helpers.meths +local exc_exec = helpers.exc_exec +local write_file = helpers.write_file + +local testdir = 'Xtest-functional-spell-spellfile.d' + +describe('spellfile', function() + before_each(function() + clear() + lfs.mkdir(testdir) + lfs.mkdir(testdir .. '/spell') + end) + after_each(function() + lfs.rmdir(testdir) + end) + -- ┌ Magic string (#VIMSPELLMAGIC) + -- │ ┌ Spell file version (#VIMSPELLVERSION) + local spellheader = 'VIMspell\050' + it('errors out when prefcond section is truncated', function() + meths.set_option('runtimepath', testdir) + write_file(testdir .. '/spell/en.ascii.spl', + -- ┌ Section identifier (#SN_PREFCOND) + -- │ ┌ Section flags (#SNF_REQUIRED or zero) + -- │ │ ┌ Section length (4 bytes, MSB first) + -- │ │ │ + spellheader .. '\003\001\000\000\000\003' + -- ┌ Number of regexes in section (2 bytes, MSB first) + -- │ ┌ Condition length (1 byte) + -- │ │ ┌ Condition regex (missing!) + .. '\000\001\001') + meths.set_option('spelllang', 'en') + eq('Vim(set):E759: Format error in spell file', + exc_exec('set spell')) + end) + it('errors out when prefcond regexp contains NUL byte', function() + meths.set_option('runtimepath', testdir) + write_file(testdir .. '/spell/en.ascii.spl', + -- ┌ Section identifier (#SN_PREFCOND) + -- │ ┌ Section flags (#SNF_REQUIRED or zero) + -- │ │ ┌ Section length (4 bytes, MSB first) + -- │ │ │ + spellheader .. '\003\001\000\000\000\008' + -- ┌ Number of regexes in section (2 bytes, MSB first) + -- │ ┌ Condition length (1 byte) + -- │ │ ┌ Condition regex + -- │ │ │ ┌ End of sections marker + .. '\000\001\005ab\000cd\255' + -- ┌ LWORDTREE tree length (4 bytes) + -- │ ┌ KWORDTREE tree length (4 bytes) + -- │ │ ┌ PREFIXTREE tree length + .. '\000\000\000\000\000\000\000\000\000\000\000\000') + meths.set_option('spelllang', 'en') + eq('Vim(set):E759: Format error in spell file', + exc_exec('set spell')) + end) +end) From 5b4f07ee86194a7c6032991102c96387581029c9 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 21:47:45 +0300 Subject: [PATCH 2/9] spellfile: Use old error This makes first test not actually show any change in behaviour. --- src/nvim/spellfile.c | 2 +- test/functional/spell/spellfile_spec.lua | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index 2c0db0694a..4909b7b14e 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -1006,7 +1006,7 @@ static int read_prefcond_section(FILE *fd, slang_T *lp) buf[0] = '^'; // always match at one position only const size_t read_byte = fread(buf + 1, 1, (size_t)n, fd); if (read_byte != (size_t)n) { - return feof(fd) ? SP_FORMERROR : SP_OTHERERROR; + return feof(fd) ? SP_TRUNCERROR : SP_OTHERERROR; } if (memchr(buf + 1, NUL, (size_t)n)) { return SP_FORMERROR; diff --git a/test/functional/spell/spellfile_spec.lua b/test/functional/spell/spellfile_spec.lua index 88f757249e..e7ad79c009 100644 --- a/test/functional/spell/spellfile_spec.lua +++ b/test/functional/spell/spellfile_spec.lua @@ -34,7 +34,7 @@ describe('spellfile', function() -- │ │ ┌ Condition regex (missing!) .. '\000\001\001') meths.set_option('spelllang', 'en') - eq('Vim(set):E759: Format error in spell file', + eq('Vim(set):E758: Truncated spell file', exc_exec('set spell')) end) it('errors out when prefcond regexp contains NUL byte', function() From ecce981dba367d61be170b535083691dd9c40cd2 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 22:02:26 +0300 Subject: [PATCH 3/9] coverity/13687: Do not allow NUL byte in region names --- src/nvim/spellfile.c | 51 ++++++++++++++++++------ test/functional/spell/spellfile_spec.lua | 18 +++++++++ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index 4909b7b14e..97a82582c0 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -494,6 +494,41 @@ typedef struct spellinfo_S { # include "spellfile.c.generated.h" #endif +/// Read n bytes from fd to buf, returning on errors +/// +/// @param[out] buf Buffer to read to, must be at least n bytes long. +/// @param[in] n Amount of bytes to read. +/// @param fd FILE* to read from. +/// +/// @return Allows to proceed if everything is OK, returns SP_TRUNCERROR if +/// there are not enough bytes, returns SP_OTHERERROR if reading failed. +#define SPELL_READ_BYTES(buf, n, fd) \ + do { \ + const size_t n__SPRB = (n); \ + FILE *const fd__SPRB = (fd); \ + char *const buf__SPRB = (buf); \ + const size_t read_bytes__SPRB = fread(buf__SPRB, 1, n__SPRB, fd__SPRB); \ + if (read_bytes__SPRB != n__SPRB) { \ + return feof(fd__SPRB) ? SP_TRUNCERROR : SP_OTHERERROR; \ + } \ + } while (0) + +/// Like #SPELL_READ_BYTES, but also error out if NUL byte was read +/// +/// @return Allows to proceed if everything is OK, returns SP_TRUNCERROR if +/// there are not enough bytes, returns SP_OTHERERROR if reading failed, +/// returns SP_FORMERROR if read out a NUL byte. +#define SPELL_READ_NONNUL_BYTES(buf, n, fd) \ + do { \ + const size_t n__SPRNB = (n); \ + FILE *const fd__SPRNB = (fd); \ + char *const buf__SPRNB = (buf); \ + SPELL_READ_BYTES(buf__SPRNB, n__SPRNB, fd__SPRNB); \ + if (memchr(buf__SPRNB, NUL, (size_t)n__SPRNB)) { \ + return SP_FORMERROR; \ + } \ + } while (0) + // Load one spell file and store the info into a slang_T. // // This is invoked in three ways: @@ -935,12 +970,10 @@ static char_u *read_cnt_string(FILE *fd, int cnt_bytes, int *cntp) // Return SP_*ERROR flags. static int read_region_section(FILE *fd, slang_T *lp, int len) { - int i; - - if (len > 16) + if (len > 16) { return SP_FORMERROR; - for (i = 0; i < len; ++i) - lp->sl_regions[i] = getc(fd); // + } + SPELL_READ_NONNUL_BYTES((char *)lp->sl_regions, (size_t)len, fd); lp->sl_regions[len] = NUL; return 0; } @@ -1004,13 +1037,7 @@ static int read_prefcond_section(FILE *fd, slang_T *lp) if (n > 0) { char buf[MAXWLEN + 1]; buf[0] = '^'; // always match at one position only - const size_t read_byte = fread(buf + 1, 1, (size_t)n, fd); - if (read_byte != (size_t)n) { - return feof(fd) ? SP_TRUNCERROR : SP_OTHERERROR; - } - if (memchr(buf + 1, NUL, (size_t)n)) { - return SP_FORMERROR; - } + SPELL_READ_NONNUL_BYTES(buf + 1, (size_t)n, fd); buf[n + 1] = NUL; lp->sl_prefprog[i] = vim_regcomp((char_u *)buf, RE_MAGIC | RE_STRING); } diff --git a/test/functional/spell/spellfile_spec.lua b/test/functional/spell/spellfile_spec.lua index e7ad79c009..4b76077605 100644 --- a/test/functional/spell/spellfile_spec.lua +++ b/test/functional/spell/spellfile_spec.lua @@ -58,4 +58,22 @@ describe('spellfile', function() eq('Vim(set):E759: Format error in spell file', exc_exec('set spell')) end) + it('errors out when region contains NUL byte', function() + meths.set_option('runtimepath', testdir) + write_file(testdir .. '/spell/en.ascii.spl', + -- ┌ Section identifier (#SN_REGION) + -- │ ┌ Section flags (#SNF_REQUIRED or zero) + -- │ │ ┌ Section length (4 bytes, MSB first) + -- │ │ │ + spellheader .. '\000\001\000\000\000\008' + -- ┌ Regions ┌ End of sections marker + .. '01234\00067\255' + -- ┌ LWORDTREE tree length (4 bytes) + -- │ ┌ KWORDTREE tree length (4 bytes) + -- │ │ ┌ PREFIXTREE tree length + .. '\000\000\000\000\000\000\000\000\000\000\000\000') + meths.set_option('spelllang', 'en') + eq('Vim(set):E759: Format error in spell file', + exc_exec('set spell')) + end) end) From 8f75b67c0733f09b8bc1d99235eb3231abc6500c Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 22:16:26 +0300 Subject: [PATCH 4/9] coverity/13688: Check for NUL bytes in salfrom --- src/nvim/spellfile.c | 4 ++-- test/functional/spell/spellfile_spec.lua | 27 +++++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index 97a82582c0..8756bee21c 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -1157,8 +1157,8 @@ static int read_sal_section(FILE *fd, slang_T *slang) if (i < ccnt) // store the char we got while checking for end of sm_lead *p++ = c; - for (++i; i < ccnt; ++i) - *p++ = getc(fd); // + SPELL_READ_NONNUL_BYTES((char *)p, (size_t)ccnt, fd); // + p += ccnt; *p++ = NUL; // diff --git a/test/functional/spell/spellfile_spec.lua b/test/functional/spell/spellfile_spec.lua index 4b76077605..05c2293c50 100644 --- a/test/functional/spell/spellfile_spec.lua +++ b/test/functional/spell/spellfile_spec.lua @@ -27,7 +27,6 @@ describe('spellfile', function() -- ┌ Section identifier (#SN_PREFCOND) -- │ ┌ Section flags (#SNF_REQUIRED or zero) -- │ │ ┌ Section length (4 bytes, MSB first) - -- │ │ │ spellheader .. '\003\001\000\000\000\003' -- ┌ Number of regexes in section (2 bytes, MSB first) -- │ ┌ Condition length (1 byte) @@ -43,7 +42,6 @@ describe('spellfile', function() -- ┌ Section identifier (#SN_PREFCOND) -- │ ┌ Section flags (#SNF_REQUIRED or zero) -- │ │ ┌ Section length (4 bytes, MSB first) - -- │ │ │ spellheader .. '\003\001\000\000\000\008' -- ┌ Number of regexes in section (2 bytes, MSB first) -- │ ┌ Condition length (1 byte) @@ -64,7 +62,6 @@ describe('spellfile', function() -- ┌ Section identifier (#SN_REGION) -- │ ┌ Section flags (#SNF_REQUIRED or zero) -- │ │ ┌ Section length (4 bytes, MSB first) - -- │ │ │ spellheader .. '\000\001\000\000\000\008' -- ┌ Regions ┌ End of sections marker .. '01234\00067\255' @@ -76,4 +73,28 @@ describe('spellfile', function() eq('Vim(set):E759: Format error in spell file', exc_exec('set spell')) end) + it('errors out when SAL section contains NUL byte', function() + meths.set_option('runtimepath', testdir) + write_file(testdir .. '/spell/en.ascii.spl', + -- ┌ Section identifier (#SN_SAL) + -- │ ┌ Section flags (#SNF_REQUIRED or zero) + -- │ │ ┌ Section length (4 bytes, MSB first) + spellheader .. '\005\001\000\000\000\008' + -- ┌ salflags + -- │ ┌ salcount (2 bytes, MSB first) + -- │ │ ┌ salfromlen (1 byte) + -- │ │ │ ┌ Special character + -- │ │ │ │┌ salfrom (should not contain NUL) + -- │ │ │ ││ ┌ saltolen + -- │ │ │ ││ │ ┌ salto + -- │ │ │ ││ │ │┌ End of sections marker + .. '\000\000\001\0024\000\0017\255' + -- ┌ LWORDTREE tree length (4 bytes) + -- │ ┌ KWORDTREE tree length (4 bytes) + -- │ │ ┌ PREFIXTREE tree length + .. '\000\000\000\000\000\000\000\000\000\000\000\000') + meths.set_option('spelllang', 'en') + eq('Vim(set):E759: Format error in spell file', + exc_exec('set spell')) + end) end) From 35584594f5cfd46e9a56d9bc3473244c437a944a Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 22:30:48 +0300 Subject: [PATCH 5/9] coverity/13689: Check file header with memcmp Not that it is actually useful (would fail in any case), but should fix coverity report. --- src/nvim/spellfile.c | 42 ++++++++++++++++++++---- test/functional/spell/spellfile_spec.lua | 8 +++++ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index 8756bee21c..9943a71a7c 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -225,6 +225,7 @@ #include #include #include +#include #include "nvim/vim.h" #include "nvim/spell_defs.h" @@ -529,6 +530,26 @@ typedef struct spellinfo_S { } \ } while (0) +/// Check that spell file starts with a magic string +/// +/// Does not check for version of the file. +/// +/// @param fd File to check. +/// +/// @return 0 in case of success, SP_TRUNCERROR if file contains not enough +/// bytes, SP_FORMERROR if it does not match magic string and +/// SP_OTHERERROR if reading file failed. +static inline int spell_check_magic_string(FILE *const fd) + FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_ALWAYS_INLINE +{ + char buf[VIMSPELLMAGICL]; + SPELL_READ_BYTES(buf, VIMSPELLMAGICL, fd); + if (memcmp(buf, VIMSPELLMAGIC, VIMSPELLMAGICL) != 0) { + return SP_FORMERROR; + } + return 0; +} + // Load one spell file and store the info into a slang_T. // // This is invoked in three ways: @@ -549,7 +570,6 @@ spell_load_file ( ) { FILE *fd; - char_u buf[VIMSPELLMAGICL]; char_u *p; int n; int len; @@ -592,12 +612,20 @@ spell_load_file ( sourcing_lnum = 0; //
: - for (size_t i = 0; i < VIMSPELLMAGICL; i++) { - buf[i] = getc(fd); // - } - if (STRNCMP(buf, VIMSPELLMAGIC, VIMSPELLMAGICL) != 0) { - EMSG(_("E757: This does not look like a spell file")); - goto endFAIL; + const int scms_ret = spell_check_magic_string(fd); + switch (scms_ret) { + case SP_FORMERROR: + case SP_TRUNCERROR: { + emsgf(_("E757: This does not look like a spell file")); + goto endFAIL; + } + case SP_OTHERERROR: { + emsgf(_("E5042: Failed to read spell file %s: %s"), + fname, strerror(ferror(fd))); + } + case 0: { + break; + } } c = getc(fd); // if (c < VIMSPELLVERSION) { diff --git a/test/functional/spell/spellfile_spec.lua b/test/functional/spell/spellfile_spec.lua index 05c2293c50..e7cd10d2ac 100644 --- a/test/functional/spell/spellfile_spec.lua +++ b/test/functional/spell/spellfile_spec.lua @@ -97,4 +97,12 @@ describe('spellfile', function() eq('Vim(set):E759: Format error in spell file', exc_exec('set spell')) end) + it('errors out when spell header contains NUL bytes', function() + meths.set_option('runtimepath', testdir) + write_file(testdir .. '/spell/en.ascii.spl', + spellheader:sub(1, -3) .. '\000\000') + meths.set_option('spelllang', 'en') + eq('Vim(set):E757: This does not look like a spell file', + exc_exec('set spell')) + end) end) From aa857f9e481770059642b9286b66c75b1d9bd758 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 22:33:45 +0300 Subject: [PATCH 6/9] spellfile: Fix memory leak --- src/nvim/spellfile.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index 9943a71a7c..d34c3a8ba1 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -500,16 +500,18 @@ typedef struct spellinfo_S { /// @param[out] buf Buffer to read to, must be at least n bytes long. /// @param[in] n Amount of bytes to read. /// @param fd FILE* to read from. +/// @param exit_code Code to run before returning. /// /// @return Allows to proceed if everything is OK, returns SP_TRUNCERROR if /// there are not enough bytes, returns SP_OTHERERROR if reading failed. -#define SPELL_READ_BYTES(buf, n, fd) \ +#define SPELL_READ_BYTES(buf, n, fd, exit_code) \ do { \ const size_t n__SPRB = (n); \ FILE *const fd__SPRB = (fd); \ char *const buf__SPRB = (buf); \ const size_t read_bytes__SPRB = fread(buf__SPRB, 1, n__SPRB, fd__SPRB); \ if (read_bytes__SPRB != n__SPRB) { \ + exit_code; \ return feof(fd__SPRB) ? SP_TRUNCERROR : SP_OTHERERROR; \ } \ } while (0) @@ -519,13 +521,14 @@ typedef struct spellinfo_S { /// @return Allows to proceed if everything is OK, returns SP_TRUNCERROR if /// there are not enough bytes, returns SP_OTHERERROR if reading failed, /// returns SP_FORMERROR if read out a NUL byte. -#define SPELL_READ_NONNUL_BYTES(buf, n, fd) \ +#define SPELL_READ_NONNUL_BYTES(buf, n, fd, exit_code) \ do { \ const size_t n__SPRNB = (n); \ FILE *const fd__SPRNB = (fd); \ char *const buf__SPRNB = (buf); \ - SPELL_READ_BYTES(buf__SPRNB, n__SPRNB, fd__SPRNB); \ + SPELL_READ_BYTES(buf__SPRNB, n__SPRNB, fd__SPRNB, exit_code); \ if (memchr(buf__SPRNB, NUL, (size_t)n__SPRNB)) { \ + exit_code; \ return SP_FORMERROR; \ } \ } while (0) @@ -543,7 +546,7 @@ static inline int spell_check_magic_string(FILE *const fd) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_ALWAYS_INLINE { char buf[VIMSPELLMAGICL]; - SPELL_READ_BYTES(buf, VIMSPELLMAGICL, fd); + SPELL_READ_BYTES(buf, VIMSPELLMAGICL, fd, ); if (memcmp(buf, VIMSPELLMAGIC, VIMSPELLMAGICL) != 0) { return SP_FORMERROR; } @@ -1001,7 +1004,7 @@ static int read_region_section(FILE *fd, slang_T *lp, int len) if (len > 16) { return SP_FORMERROR; } - SPELL_READ_NONNUL_BYTES((char *)lp->sl_regions, (size_t)len, fd); + SPELL_READ_NONNUL_BYTES((char *)lp->sl_regions, (size_t)len, fd, ); lp->sl_regions[len] = NUL; return 0; } @@ -1065,7 +1068,7 @@ static int read_prefcond_section(FILE *fd, slang_T *lp) if (n > 0) { char buf[MAXWLEN + 1]; buf[0] = '^'; // always match at one position only - SPELL_READ_NONNUL_BYTES(buf + 1, (size_t)n, fd); + SPELL_READ_NONNUL_BYTES(buf + 1, (size_t)n, fd, ); buf[n + 1] = NUL; lp->sl_prefprog[i] = vim_regcomp((char_u *)buf, RE_MAGIC | RE_STRING); } @@ -1185,7 +1188,8 @@ static int read_sal_section(FILE *fd, slang_T *slang) if (i < ccnt) // store the char we got while checking for end of sm_lead *p++ = c; - SPELL_READ_NONNUL_BYTES((char *)p, (size_t)ccnt, fd); // + SPELL_READ_NONNUL_BYTES( // + (char *)p, (size_t)ccnt, fd, xfree(smp->sm_lead)); p += ccnt; *p++ = NUL; From eb3663eb1000bdf9b5c754614921001b21a2cf03 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 22:39:23 +0300 Subject: [PATCH 7/9] spellfile: Fix clint errors --- src/nvim/spellfile.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index d34c3a8ba1..f4acb37a7a 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -546,7 +546,7 @@ static inline int spell_check_magic_string(FILE *const fd) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_ALWAYS_INLINE { char buf[VIMSPELLMAGICL]; - SPELL_READ_BYTES(buf, VIMSPELLMAGICL, fd, ); + SPELL_READ_BYTES(buf, VIMSPELLMAGICL, fd, ;); if (memcmp(buf, VIMSPELLMAGIC, VIMSPELLMAGICL) != 0) { return SP_FORMERROR; } @@ -1004,7 +1004,7 @@ static int read_region_section(FILE *fd, slang_T *lp, int len) if (len > 16) { return SP_FORMERROR; } - SPELL_READ_NONNUL_BYTES((char *)lp->sl_regions, (size_t)len, fd, ); + SPELL_READ_NONNUL_BYTES((char *)lp->sl_regions, (size_t)len, fd, ;); lp->sl_regions[len] = NUL; return 0; } @@ -1056,7 +1056,7 @@ static int read_prefcond_section(FILE *fd, slang_T *lp) lp->sl_prefprog = xcalloc(cnt, sizeof(regprog_T *)); lp->sl_prefixcnt = cnt; - for (int i = 0; i < cnt; ++i) { + for (int i = 0; i < cnt; i++) { // : const int n = getc(fd); // if (n < 0 || n >= MAXWLEN) { @@ -1068,7 +1068,7 @@ static int read_prefcond_section(FILE *fd, slang_T *lp) if (n > 0) { char buf[MAXWLEN + 1]; buf[0] = '^'; // always match at one position only - SPELL_READ_NONNUL_BYTES(buf + 1, (size_t)n, fd, ); + SPELL_READ_NONNUL_BYTES(buf + 1, (size_t)n, fd, ;); buf[n + 1] = NUL; lp->sl_prefprog[i] = vim_regcomp((char_u *)buf, RE_MAGIC | RE_STRING); } From fa7ace446e724f888c815fe177c7b6e7b8057b7d Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 23:38:05 +0300 Subject: [PATCH 8/9] coverity/56795: Fix NULL dereference in :syn keyword non-printable Bug was introduced 3 years earlier, in 13848aa: NULL keyword_copy was incorrectly treated as an indicator of OOM. --- src/nvim/syntax.c | 124 ++++++++++++------------ test/functional/ex_cmds/syntax_spec.lua | 17 ++++ 2 files changed, 78 insertions(+), 63 deletions(-) create mode 100644 test/functional/ex_cmds/syntax_spec.lua diff --git a/src/nvim/syntax.c b/src/nvim/syntax.c index e36b00d770..1ed65ec52a 100644 --- a/src/nvim/syntax.c +++ b/src/nvim/syntax.c @@ -4246,83 +4246,81 @@ static void syn_cmd_keyword(exarg_T *eap, int syncing) if (rest != NULL) { syn_id = syn_check_group(arg, (int)(group_name_end - arg)); - if (syn_id != 0) - /* allocate a buffer, for removing backslashes in the keyword */ + if (syn_id != 0) { + // Allocate a buffer, for removing backslashes in the keyword. keyword_copy = xmalloc(STRLEN(rest) + 1); - syn_opt_arg.flags = 0; - syn_opt_arg.keyword = TRUE; - syn_opt_arg.sync_idx = NULL; - syn_opt_arg.has_cont_list = FALSE; - syn_opt_arg.cont_in_list = NULL; - syn_opt_arg.next_list = NULL; - - /* - * The options given apply to ALL keywords, so all options must be - * found before keywords can be created. - * 1: collect the options and copy the keywords to keyword_copy. - */ - cnt = 0; - p = keyword_copy; - for (; rest != NULL && !ends_excmd(*rest); rest = skipwhite(rest)) { - rest = get_syn_options(rest, &syn_opt_arg, &conceal_char); - if (rest == NULL || ends_excmd(*rest)) - break; - /* Copy the keyword, removing backslashes, and add a NUL. */ - while (*rest != NUL && !ascii_iswhite(*rest)) { - if (*rest == '\\' && rest[1] != NUL) - ++rest; - *p++ = *rest++; - } - *p++ = NUL; - ++cnt; } + if (keyword_copy != NULL) { + syn_opt_arg.flags = 0; + syn_opt_arg.keyword = true; + syn_opt_arg.sync_idx = NULL; + syn_opt_arg.has_cont_list = false; + syn_opt_arg.cont_in_list = NULL; + syn_opt_arg.next_list = NULL; - if (!eap->skip) { - /* Adjust flags for use of ":syn include". */ - syn_incl_toplevel(syn_id, &syn_opt_arg.flags); - - /* - * 2: Add an entry for each keyword. - */ - for (kw = keyword_copy; --cnt >= 0; kw += STRLEN(kw) + 1) { - for (p = vim_strchr(kw, '[');; ) { - if (p != NULL) - *p = NUL; - add_keyword(kw, syn_id, syn_opt_arg.flags, - syn_opt_arg.cont_in_list, - syn_opt_arg.next_list, conceal_char); - if (p == NULL) - break; - if (p[1] == NUL) { - EMSG2(_("E789: Missing ']': %s"), kw); - goto error; + // The options given apply to ALL keywords, so all options must be + // found before keywords can be created. + // 1: collect the options and copy the keywords to keyword_copy. + cnt = 0; + p = keyword_copy; + for (; rest != NULL && !ends_excmd(*rest); rest = skipwhite(rest)) { + rest = get_syn_options(rest, &syn_opt_arg, &conceal_char); + if (rest == NULL || ends_excmd(*rest)) { + break; + } + // Copy the keyword, removing backslashes, and add a NUL. + while (*rest != NUL && !ascii_iswhite(*rest)) { + if (*rest == '\\' && rest[1] != NUL) { + rest++; } - if (p[1] == ']') { - if (p[2] != NUL) { - EMSG3(_("E890: trailing char after ']': %s]%s"), - kw, &p[2]); + *p++ = *rest++; + } + *p++ = NUL; + cnt++; + } + + if (!eap->skip) { + // Adjust flags for use of ":syn include". + syn_incl_toplevel(syn_id, &syn_opt_arg.flags); + + // 2: Add an entry for each keyword. + for (kw = keyword_copy; --cnt >= 0; kw += STRLEN(kw) + 1) { + for (p = vim_strchr(kw, '[');; ) { + if (p != NULL) { + *p = NUL; + } + add_keyword(kw, syn_id, syn_opt_arg.flags, + syn_opt_arg.cont_in_list, + syn_opt_arg.next_list, conceal_char); + if (p == NULL) { + break; + } + if (p[1] == NUL) { + emsgf(_("E789: Missing ']': %s"), kw); goto error; } - kw = p + 1; - break; // skip over the "]" - } - if (has_mbyte) { - int l = (*mb_ptr2len)(p + 1); + if (p[1] == ']') { + if (p[2] != NUL) { + emsgf(_("E890: trailing char after ']': %s]%s"), + kw, &p[2]); + goto error; + } + kw = p + 1; + break; // skip over the "]" + } + const int l = (*mb_ptr2len)(p + 1); memmove(p, p + 1, l); p += l; - } else { - p[0] = p[1]; - ++p; } } } - } error: - xfree(keyword_copy); - xfree(syn_opt_arg.cont_in_list); - xfree(syn_opt_arg.next_list); + xfree(keyword_copy); + xfree(syn_opt_arg.cont_in_list); + xfree(syn_opt_arg.next_list); + } } if (rest != NULL) diff --git a/test/functional/ex_cmds/syntax_spec.lua b/test/functional/ex_cmds/syntax_spec.lua new file mode 100644 index 0000000000..c9e96703de --- /dev/null +++ b/test/functional/ex_cmds/syntax_spec.lua @@ -0,0 +1,17 @@ +local helpers = require('test.functional.helpers')(after_each) + +local eq = helpers.eq +local clear = helpers.clear +local exc_exec = helpers.exc_exec + +describe(':syntax', function() + before_each(clear) + + describe('keyword', function() + it('does not crash when group name contains unprintable characters', + function() + eq('Vim(syntax):E669: Unprintable character in group name', + exc_exec('syntax keyword \024 foo bar')) + end) + end) +end) From ebe50519775081565b66e18a471473e46f713442 Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 9 Apr 2017 23:46:38 +0300 Subject: [PATCH 9/9] spellfile: Fix SAL sections reading --- src/nvim/spellfile.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/nvim/spellfile.c b/src/nvim/spellfile.c index f4acb37a7a..1da71dc4f9 100644 --- a/src/nvim/spellfile.c +++ b/src/nvim/spellfile.c @@ -1123,7 +1123,6 @@ static int read_rep_section(FILE *fd, garray_T *gap, int16_t *first) // Return SP_*ERROR flags. static int read_sal_section(FILE *fd, slang_T *slang) { - int i; int cnt; garray_T *gap; salitem_T *smp; @@ -1133,13 +1132,16 @@ static int read_sal_section(FILE *fd, slang_T *slang) slang->sl_sofo = false; - i = getc(fd); // - if (i & SAL_F0LLOWUP) + const int flags = getc(fd); // + if (flags & SAL_F0LLOWUP) { slang->sl_followup = true; - if (i & SAL_COLLAPSE) + } + if (flags & SAL_COLLAPSE) { slang->sl_collapse = true; - if (i & SAL_REM_ACCENTS) + } + if (flags & SAL_REM_ACCENTS) { slang->sl_rem_accents = true; + } cnt = get2c(fd); // if (cnt < 0) @@ -1159,7 +1161,8 @@ static int read_sal_section(FILE *fd, slang_T *slang) smp->sm_lead = p; // Read up to the first special char into sm_lead. - for (i = 0; i < ccnt; ++i) { + int i = 0; + for (; i < ccnt; ++i) { c = getc(fd); // if (vim_strchr((char_u *)"0123456789(-<^$", c) != NULL) break; @@ -1185,12 +1188,17 @@ static int read_sal_section(FILE *fd, slang_T *slang) // Any following chars go in sm_rules. smp->sm_rules = p; - if (i < ccnt) + if (i < ccnt) { // store the char we got while checking for end of sm_lead *p++ = c; - SPELL_READ_NONNUL_BYTES( // - (char *)p, (size_t)ccnt, fd, xfree(smp->sm_lead)); - p += ccnt; + } + i++; + if (i < ccnt) { + SPELL_READ_NONNUL_BYTES( // + (char *)p, (size_t)(ccnt - i), fd, xfree(smp->sm_lead)); + p += (ccnt - i); + i = ccnt; + } *p++ = NUL; //