Merge #5996 from justinmk/coverity-133845

xstrlcat() + coverity fixes

runtime/doc/helphelp.txt

@@ -185,7 +185,7 @@ command: >
 <
 
 					*:helpt* *:helptags*
-				*E154* *E150* *E151* *E152* *E153* *E670*
+			*E154* *E150* *E151* *E152* *E153* *E670* *E856*
 :helpt[ags] [++t] {dir}
 			Generate the help tags file(s) for directory {dir}.
 			When {dir} is ALL then all "doc" directories in

src/clint.py

@@ -3166,11 +3166,15 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension,
     # Check if some verboten C functions are being used.
     if Search(r'\bsprintf\b', line):
         error(filename, linenum, 'runtime/printf', 5,
-              'Never use sprintf.  Use snprintf instead.')
-    match = Search(r'\b(strcpy|strcat)\b', line)
+              'Use snprintf instead of sprintf.')
+    match = Search(r'\b(STRCPY|strcpy)\b', line)
     if match:
         error(filename, linenum, 'runtime/printf', 4,
-              'Almost always, snprintf is better than %s' % match.group(1))
+              'Use xstrlcpy or snprintf instead of %s' % match.group(1))
+    match = Search(r'\b(STRNCAT|strncat)\b', line)
+    if match:
+        error(filename, linenum, 'runtime/printf', 4,
+              'Use xstrlcat instead of %s' % match.group(1))
 
     # Check for suspicious usage of "if" like
     # } if (a == b) {

src/nvim/ex_cmds.c

@@ -1408,8 +1408,8 @@ char_u *make_filter_cmd(char_u *cmd, char_u *itmp, char_u *otmp)
   }
 
   if (itmp != NULL) {
-    strncat(buf, " < ", len - 1);
-    strncat(buf, (const char *)itmp, len - 1);
+    xstrlcat(buf, " < ", len - 1);
+    xstrlcat(buf, (const char *)itmp, len - 1);
   }
 #else
   // For shells that don't understand braces around commands, at least allow
@@ -1425,13 +1425,13 @@ char_u *make_filter_cmd(char_u *cmd, char_u *itmp, char_u *otmp)
         *p = NUL;
       }
     }
-    strncat(buf, " < ", len);
-    strncat(buf, (const char *)itmp, len);
+    xstrlcat(buf, " < ", len);
+    xstrlcat(buf, (const char *)itmp, len);
     if (*p_shq == NUL) {
       const char *const p = strchr((const char *)cmd, '|');
       if (p != NULL) {
-        strncat(buf, " ", len - 1);  // Insert a space before the '|' for DOS
-        strncat(buf, p, len - 1);
+        xstrlcat(buf, " ", len - 1);  // Insert a space before the '|' for DOS
+        xstrlcat(buf, p, len - 1);
       }
     }
   }
@@ -4812,9 +4812,13 @@ void fix_help_buffer(void)
           vimconv_T vc;
           char_u      *cp;
 
-          /* Find all "doc/ *.txt" files in this directory. */
-          add_pathsep((char *)NameBuff);
-          STRCAT(NameBuff, "doc/*.??[tx]");
+          // Find all "doc/ *.txt" files in this directory.
+          if (!add_pathsep((char *)NameBuff)
+              || STRLCAT(NameBuff, "doc/*.??[tx]",
+                         sizeof(NameBuff)) >= MAXPATHL) {
+            EMSG(_(e_fnametoolong));
+            continue;
+          }
 
           // Note: We cannot just do `&NameBuff` because it is a statically sized array
           //       so `NameBuff == &NameBuff` according to C semantics.
@@ -4977,8 +4981,12 @@ static void helptags_one(char_u *dir, char_u *ext, char_u *tagfname,
 
   // Find all *.txt files.
   size_t dirlen = STRLCPY(NameBuff, dir, sizeof(NameBuff));
-  STRCAT(NameBuff, "/**/*");  // NOLINT
-  STRCAT(NameBuff, ext);
+  if (dirlen >= MAXPATHL
+      || STRLCAT(NameBuff, "/**/*", sizeof(NameBuff)) >= MAXPATHL  // NOLINT
+      || STRLCAT(NameBuff, ext, sizeof(NameBuff)) >= MAXPATHL) {
+    EMSG(_(e_fnametoolong));
+    return;
+  }
 
   // Note: We cannot just do `&NameBuff` because it is a statically sized array
   //       so `NameBuff == &NameBuff` according to C semantics.
@@ -4991,13 +4999,16 @@ static void helptags_one(char_u *dir, char_u *ext, char_u *tagfname,
     return;
   }
 
-  /*
-   * Open the tags file for writing.
-   * Do this before scanning through all the files.
-   */
-  STRLCPY(NameBuff, dir, sizeof(NameBuff));
-  add_pathsep((char *)NameBuff);
-  STRNCAT(NameBuff, tagfname, sizeof(NameBuff) - dirlen - 2);
+  //
+  // Open the tags file for writing.
+  // Do this before scanning through all the files.
+  //
+  memcpy(NameBuff, dir, dirlen + 1);
+  if (!add_pathsep((char *)NameBuff)
+      || STRLCAT(NameBuff, tagfname, sizeof(NameBuff)) >= MAXPATHL) {
+    EMSG(_(e_fnametoolong));
+    return;
+  }
   fd_tags = mch_fopen((char *)NameBuff, "w");
   if (fd_tags == NULL) {
     EMSG2(_("E152: Cannot open %s for writing"), NameBuff);
@@ -5171,8 +5182,12 @@ static void do_helptags(char_u *dirname, bool add_help_tags)
 
   // Get a list of all files in the help directory and in subdirectories.
   STRLCPY(NameBuff, dirname, sizeof(NameBuff));
-  add_pathsep((char *)NameBuff);
-  STRCAT(NameBuff, "**");
+  if (!add_pathsep((char *)NameBuff)
+      || STRLCAT(NameBuff, "**", sizeof(NameBuff)) >= MAXPATHL) {
+    EMSG(_(e_fnametoolong));
+    xfree(dirname);
+    return;
+  }
 
   // Note: We cannot just do `&NameBuff` because it is a statically sized array
   //       so `NameBuff == &NameBuff` according to C semantics.

src/nvim/file_search.c

@@ -193,7 +193,6 @@ typedef struct ff_search_ctx_T {
 
 static char_u e_pathtoolong[] = N_("E854: path too long for completion");
 
-
 /*
  * Initialization routine for vim_findfile().
  *

src/nvim/fileio.c

@@ -4982,8 +4982,8 @@ buf_check_timestamp (
     set_vim_var_string(VV_WARNINGMSG, tbuf, -1);
     if (can_reload) {
       if (*mesg2 != NUL) {
-        strncat(tbuf, "\n", tbuf_len - 1);
-        strncat(tbuf, mesg2, tbuf_len - 1);
+        xstrlcat(tbuf, "\n", tbuf_len - 1);
+        xstrlcat(tbuf, mesg2, tbuf_len - 1);
       }
       if (do_dialog(VIM_WARNING, (char_u *) _("Warning"), (char_u *) tbuf,
                     (char_u *) _("&OK\n&Load File"), 1, NULL, true) == 2) {
@@ -4991,8 +4991,8 @@ buf_check_timestamp (
       }
     } else if (State > NORMAL_BUSY || (State & CMDLINE) || already_warned) {
       if (*mesg2 != NUL) {
-        strncat(tbuf, "; ", tbuf_len);
-        strncat(tbuf, mesg2, tbuf_len);
+        xstrlcat(tbuf, "; ", tbuf_len - 1);
+        xstrlcat(tbuf, mesg2, tbuf_len - 1);
       }
       EMSG(tbuf);
       retval = 2;

src/nvim/globals.h

@@ -1215,6 +1215,7 @@ EXTERN char_u e_invalidreg[] INIT(= N_("E850: Invalid register name"));
 EXTERN char_u e_dirnotf[] INIT(= N_(
     "E919: Directory not found in '%s': \"%s\""));
 EXTERN char_u e_unsupportedoption[] INIT(= N_("E519: Option not supported"));
+EXTERN char_u e_fnametoolong[] INIT(= N_("E856: Filename too long"));
 
 
 EXTERN char top_bot_msg[] INIT(= N_("search hit TOP, continuing at BOTTOM"));

src/nvim/hardcopy.c

@@ -1544,8 +1544,8 @@ static int prt_find_resource(char *name, struct prt_ps_resource_S *resource)
   /* Look for named resource file in runtimepath */
   STRCPY(buffer, "print");
   add_pathsep((char *)buffer);
-  vim_strcat(buffer, (char_u *)name, MAXPATHL);
-  vim_strcat(buffer, (char_u *)".ps", MAXPATHL);
+  xstrlcat((char *)buffer, name, MAXPATHL);
+  xstrlcat((char *)buffer, ".ps", MAXPATHL);
   resource->filename[0] = NUL;
   retval = (do_in_runtimepath(buffer, 0, prt_resource_name, resource->filename)
             && resource->filename[0] != NUL);

src/nvim/memory.c

@@ -389,6 +389,33 @@ size_t xstrlcpy(char *restrict dst, const char *restrict src, size_t size)
   return ret;
 }
 
+/// xstrlcat - Appends string src to the end of dst.
+///
+/// Compatible with *BSD strlcat: Appends at most (dstsize - strlen(dst) - 1)
+/// characters. dst will be NUL-terminated.
+///
+/// Note: Replaces `vim_strcat`.
+///
+/// @param dst      Where to copy the string to
+/// @param src      Where to copy the string from
+/// @param dstsize  Size of destination buffer, must be greater than 0
+/// @return         strlen(src) + MIN(dstsize, strlen(initial dst)).
+///                 If retval >= dstsize, truncation occurs.
+size_t xstrlcat(char *restrict dst, const char *restrict src, size_t dstsize)
+  FUNC_ATTR_NONNULL_ALL
+{
+  assert(dstsize > 0);
+  size_t srclen = strlen(src);
+  size_t dstlen = strlen(dst);
+  size_t ret = srclen + dstlen;  // Total string length (excludes NUL)
+  if (srclen) {
+    size_t len = (ret >= dstsize) ? dstsize - 1 : ret;
+    memcpy(dst + dstlen, src, len - dstlen);
+    dst[len] = '\0';
+  }
+  return ret;  // Does not include NUL.
+}
+
 /// strdup() wrapper
 ///
 /// @see {xmalloc}

src/nvim/message.c

@@ -381,20 +381,17 @@ static int other_sourcing_name(void)
   return FALSE;
 }
 
-/*
- * Get the message about the source, as used for an error message.
- * Returns an allocated string with room for one more character.
- * Returns NULL when no message is to be given.
- */
+/// Get the message about the source, as used for an error message.
+/// Returns an allocated string with room for one more character.
+/// Returns NULL when no message is to be given.
 static char_u *get_emsg_source(void)
 {
-  char_u      *Buf, *p;
-
   if (sourcing_name != NULL && other_sourcing_name()) {
-    p = (char_u *)_("Error detected while processing %s:");
-    Buf = xmalloc(STRLEN(sourcing_name) + STRLEN(p));
-    sprintf((char *)Buf, (char *)p, sourcing_name);
-    return Buf;
+    char_u *p = (char_u *)_("Error detected while processing %s:");
+    size_t len = STRLEN(sourcing_name) + STRLEN(p) + 1;
+    char_u *buf = xmalloc(len);
+    snprintf((char *)buf, len, (char *)p, sourcing_name);
+    return buf;
   }
   return NULL;
 }

src/nvim/misc1.c

@@ -2507,8 +2507,9 @@ void msgmore(long n)
         vim_snprintf((char *)msg_buf, MSG_BUF_LEN,
             _("%" PRId64 " fewer lines"), (int64_t)pn);
     }
-    if (got_int)
-      vim_strcat(msg_buf, (char_u *)_(" (Interrupted)"), MSG_BUF_LEN);
+    if (got_int) {
+      xstrlcat((char *)msg_buf, _(" (Interrupted)"), MSG_BUF_LEN);
+    }
     if (msg(msg_buf)) {
       set_keep_msg(msg_buf, 0);
       keep_msg_more = TRUE;

src/nvim/normal.c

@@ -1130,6 +1130,7 @@ static int normal_execute(VimState *state, int key)
       start_selection();
       unshift_special(&s->ca);
       s->idx = find_command(s->ca.cmdchar);
+      assert(s->idx >= 0);
     } else if ((nv_cmds[s->idx].cmd_flags & NV_SSS)
                && (mod_mask & MOD_MASK_SHIFT)) {
       start_selection();

src/nvim/os/os_defs.h

@@ -16,7 +16,7 @@
 #define BASENAMELEN (NAME_MAX - 5)
 
 // Use the system path length if it makes sense.
-#if defined(PATH_MAX) && (PATH_MAX > 1000)
+#if defined(PATH_MAX) && (PATH_MAX > 1024)
 # define MAXPATHL PATH_MAX
 #else
 # define MAXPATHL 1024

src/nvim/path.c

@@ -391,15 +391,22 @@ char *concat_fnames_realloc(char *fname1, const char *fname2, bool sep)
                           fname2, len2, sep);
 }
 
-/*
- * Add a path separator to a file name, unless it already ends in a path
- * separator.
- */
-void add_pathsep(char *p)
+/// Adds a path separator to a filename, unless it already ends in one.
+///
+/// @return `true` if the path separator was added or already existed.
+///         `false` if the filename is too long.
+bool add_pathsep(char *p)
   FUNC_ATTR_NONNULL_ALL
 {
-  if (*p != NUL && !after_pathsep(p, p + strlen(p)))
-    strcat(p, PATHSEPSTR);
+  const size_t len = strlen(p);
+  if (*p != NUL && !after_pathsep(p, p + len)) {
+    const size_t pathsep_len = sizeof(PATHSEPSTR);
+    if (len > MAXPATHL - pathsep_len) {
+      return false;
+    }
+    memcpy(p + len, PATHSEPSTR, pathsep_len);
+  }
+  return true;
 }
 
 /// Get an allocated copy of the full path to a file.

src/nvim/quickfix.c

@@ -2126,7 +2126,7 @@ static void qf_msg(qf_info_T *qi, int which, char *lead)
        memset(buf + len, ' ', 34 - len);
        buf[34] = NUL;
     }
-    vim_strcat(buf, (char_u *)title, IOSIZE);
+    xstrlcat((char *)buf, title, IOSIZE);
   }
   trunc_string(buf, buf, (int)Columns - 1, IOSIZE);
   msg(buf);

src/nvim/strings.c

@@ -344,24 +344,6 @@ void del_trailing_spaces(char_u *ptr)
     *q = NUL;
 }
 
-/*
- * Like strcat(), but make sure the result fits in "tosize" bytes and is
- * always NUL terminated.
- */
-void vim_strcat(char_u *restrict to, const char_u *restrict from,
-                size_t tosize)
-  FUNC_ATTR_NONNULL_ALL
-{
-  size_t tolen = STRLEN(to);
-  size_t fromlen = STRLEN(from);
-
-  if (tolen + fromlen + 1 > tosize) {
-    memcpy(to + tolen, from, tosize - tolen - 1);
-    to[tosize - 1] = NUL;
-  } else
-    STRCPY(to + tolen, from);
-}
-
 #if (!defined(HAVE_STRCASECMP) && !defined(HAVE_STRICMP))
 /*
  * Compare two strings, ignoring case, using current locale.

src/nvim/syntax.c

@@ -6902,8 +6902,8 @@ static int highlight_list_arg(int id, int didh, int type, int iarg, char_u *sarg
       for (i = 0; hl_attr_table[i] != 0; ++i) {
         if (iarg & hl_attr_table[i]) {
           if (buf[0] != NUL)
-            vim_strcat(buf, (char_u *)",", 100);
-          vim_strcat(buf, (char_u *)hl_name_table[i], 100);
+            xstrlcat((char *)buf, ",", 100);
+          xstrlcat((char *)buf, hl_name_table[i], 100);
           iarg &= ~hl_attr_table[i];                /* don't want "inverse" */
         }
       }

src/nvim/vim.h

@@ -269,6 +269,7 @@ enum {
 
 #define STRCAT(d, s)        strcat((char *)(d), (char *)(s))
 #define STRNCAT(d, s, n)    strncat((char *)(d), (char *)(s), (size_t)(n))
+#define STRLCAT(d, s, n)    xstrlcat((char *)(d), (char *)(s), (size_t)(n))
 
 # define vim_strpbrk(s, cs) (char_u *)strpbrk((char *)(s), (char *)(cs))
 

test/functional/ui/screen.lua

@@ -613,7 +613,7 @@ function Screen:_pprint_attrs(attrs)
     return table.concat(items, ", ")
 end
 
-function backward_find_meaningful(tbl, from)  -- luacheck: ignore
+local function backward_find_meaningful(tbl, from)  -- luacheck: no unused
   for i = from or #tbl, 1, -1 do
     if tbl[i] ~= ' ' then
       return i + 1