Merge #6488 from ZyX-I/coverity-fixes

src/nvim/spellfile.c

@@ -225,6 +225,7 @@
 #include <stdio.h>
 #include <stdint.h>
 #include <wctype.h>
+#include <strings.h>
 
 #include "nvim/vim.h"
 #include "nvim/spell_defs.h"
@@ -267,7 +268,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!
@@ -494,6 +495,64 @@ 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.
+/// @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, 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)
+
+/// 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, 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, exit_code); \
+      if (memchr(buf__SPRNB, NUL, (size_t)n__SPRNB)) { \
+        exit_code; \
+        return SP_FORMERROR; \
+      } \
+    } 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:
@@ -514,9 +573,7 @@ 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,11 +615,20 @@ spell_load_file (
   sourcing_lnum = 0;
 
   // <HEADER>: <fileID>
-  for (i = 0; i < VIMSPELLMAGICL; ++i)
+  const int scms_ret = spell_check_magic_string(fd);
-    buf[i] = getc(fd);                                  // <fileID>
+  switch (scms_ret) {
-  if (STRNCMP(buf, VIMSPELLMAGIC, VIMSPELLMAGICL) != 0) {
+    case SP_FORMERROR:
-    EMSG(_("E757: This does not look like a spell file"));
+    case SP_TRUNCERROR: {
-    goto endFAIL;
+      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);                                         // <versionnr>
   if (c < VIMSPELLVERSION) {
@@ -935,12 +1001,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);                       // <regionname>
+  SPELL_READ_NONNUL_BYTES((char *)lp->sl_regions, (size_t)len, fd, ;);
   lp->sl_regions[len] = NUL;
   return 0;
 }
@@ -983,35 +1047,30 @@ 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];
-
   // <prefcondcnt> <prefcond> ...
-  cnt = get2c(fd);                                      // <prefcondcnt>
+  const int cnt = get2c(fd);  // <prefcondcnt>
-  if (cnt <= 0)
+  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++) {
     // <prefcond> : <condlen> <condstr>
-    n = getc(fd);                                       // <condlen>
+    const int n = getc(fd);  // <condlen>
-    if (n < 0 || n >= MAXWLEN)
+    if (n < 0 || n >= MAXWLEN) {
       return SP_FORMERROR;
+    }
 
     // When <condlen> 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
+      char buf[MAXWLEN + 1];
-      p = buf + 1;
+      buf[0] = '^';  // always match at one position only
-      while (n-- > 0)
+      SPELL_READ_NONNUL_BYTES(buf + 1, (size_t)n, fd, ;);
-        *p++ = getc(fd);                                // <condstr>
+      buf[n + 1] = NUL;
-      *p = NUL;
+      lp->sl_prefprog[i] = vim_regcomp((char_u *)buf, RE_MAGIC | RE_STRING);
-      lp->sl_prefprog[i] = vim_regcomp(buf, RE_MAGIC + RE_STRING);
     }
   }
   return 0;
@@ -1064,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;
@@ -1074,13 +1132,16 @@ static int read_sal_section(FILE *fd, slang_T *slang)
 
   slang->sl_sofo = false;
 
-  i = getc(fd);                                 // <salflags>
+  const int flags = getc(fd);                   // <salflags>
-  if (i & SAL_F0LLOWUP)
+  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);                              // <salcount>
   if (cnt < 0)
@@ -1100,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);                             // <salfrom>
       if (vim_strchr((char_u *)"0123456789(-<^$", c) != NULL)
         break;
@@ -1126,11 +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;
-    for (++i; i < ccnt; ++i)
+    }
-      *p++ = getc(fd);                          // <salfrom>
+    i++;
+    if (i < ccnt) {
+      SPELL_READ_NONNUL_BYTES(                  // <salfrom>
+          (char *)p, (size_t)(ccnt - i), fd, xfree(smp->sm_lead));
+      p += (ccnt - i);
+      i = ccnt;
+    }
     *p++ = NUL;
 
     // <saltolen> <salto>

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)
+    if (syn_id != 0) {
-      /* allocate a buffer, for removing backslashes in the keyword */
+      // 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) {
+      // The options given apply to ALL keywords, so all options must be
-      /* Adjust flags for use of ":syn include". */
+      // found before keywords can be created.
-      syn_incl_toplevel(syn_id, &syn_opt_arg.flags);
+      // 1: collect the options and copy the keywords to keyword_copy.
-
+      cnt = 0;
-      /*
+      p = keyword_copy;
-       * 2: Add an entry for each keyword.
+      for (; rest != NULL && !ends_excmd(*rest); rest = skipwhite(rest)) {
-       */
+        rest = get_syn_options(rest, &syn_opt_arg, &conceal_char);
-      for (kw = keyword_copy; --cnt >= 0; kw += STRLEN(kw) + 1) {
+        if (rest == NULL || ends_excmd(*rest)) {
-        for (p = vim_strchr(kw, '[');; ) {
+          break;
-          if (p != NULL)
+        }
-            *p = NUL;
+        // Copy the keyword, removing backslashes, and add a NUL.
-          add_keyword(kw, syn_id, syn_opt_arg.flags,
+        while (*rest != NUL && !ascii_iswhite(*rest)) {
-              syn_opt_arg.cont_in_list,
+          if (*rest == '\\' && rest[1] != NUL) {
-              syn_opt_arg.next_list, conceal_char);
+            rest++;
-          if (p == NULL)
-            break;
-          if (p[1] == NUL) {
-            EMSG2(_("E789: Missing ']': %s"), kw);
-            goto error;
           }
-          if (p[1] == ']') {
+          *p++ = *rest++;
-            if (p[2] != NUL) {
+        }
-              EMSG3(_("E890: trailing char after ']': %s]%s"),
+        *p++ = NUL;
-                    kw, &p[2]);
+        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;
+            if (p[1] == ']') {
-            break;   // skip over the "]"
+              if (p[2] != NUL) {
-          }
+                emsgf(_("E890: trailing char after ']': %s]%s"),
-          if (has_mbyte) {
+                      kw, &p[2]);
-            int l = (*mb_ptr2len)(p + 1);
+                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(keyword_copy);
-    xfree(syn_opt_arg.cont_in_list);
+      xfree(syn_opt_arg.cont_in_list);
-    xfree(syn_opt_arg.next_list);
+      xfree(syn_opt_arg.next_list);
+    }
   }
 
   if (rest != NULL)

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)

test/functional/spell/spellfile_spec.lua

@@ -0,0 +1,108 @@
+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):E758: Truncated 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)
+  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)
+  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)
+  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)