vim-patch:9.1.1467: too many strlen() calls (#34572)

Problem:  too many strlen() calls
Solution: Change expand_env() to return string length
          (John Marriott)

This commit does the following changes:
- In expand_env_esc():
  - return the length of the returned dst string.
  - refactor to remove some calls to STRLEN() and STRCAT()
  - add check for out-of-memory condition.
- Change call sites in various source files to use the return value

closes: vim/vim#17561

fff0132399

Co-authored-by: John Marriott <basilisk@internode.on.net>

src/nvim/file_search.c

@@ -1449,11 +1449,10 @@ char *find_file_in_path_option(char *ptr, size_t len, int options, int first, ch
     // copy file name into NameBuff, expanding environment variables
     char save_char = ptr[len];
     ptr[len] = NUL;
-    expand_env_esc(ptr, NameBuff, MAXPATHL, false, true, NULL);
+    file_to_findlen = expand_env_esc(ptr, NameBuff, MAXPATHL, false, true, NULL);
     ptr[len] = save_char;
 
     xfree(*file_to_find);
-    file_to_findlen = strlen(NameBuff);
     *file_to_find = xmemdupz(NameBuff, file_to_findlen);
     if (options & FNAME_UNESC) {
       // Change all "\ " to " ".

src/nvim/fileio.c

@@ -58,6 +58,7 @@
 #include "nvim/os/fs_defs.h"
 #include "nvim/os/input.h"
 #include "nvim/os/os.h"
+#include "nvim/os/os_defs.h"
 #include "nvim/os/time.h"
 #include "nvim/path.h"
 #include "nvim/pos_defs.h"
@@ -3275,7 +3276,7 @@ static void vim_mktempdir(void)
   mode_t umask_save = umask(0077);
   for (size_t i = 0; i < ARRAY_SIZE(temp_dirs); i++) {
     // Expand environment variables, leave room for "/tmp/nvim.<user>/XXXXXX/999999999".
-    expand_env((char *)temp_dirs[i], tmp, TEMP_FILE_PATH_MAXLEN - 64);
+    size_t tmplen = expand_env((char *)temp_dirs[i], tmp, TEMP_FILE_PATH_MAXLEN - 64);
     if (!os_isdir(tmp)) {
       if (strequal("$TMPDIR", temp_dirs[i])) {
         if (!os_env_exists("TMPDIR", true)) {
@@ -3288,9 +3289,13 @@ static void vim_mktempdir(void)
     }
 
     // "/tmp/" exists, now try to create "/tmp/nvim.<user>/".
-    add_pathsep(tmp);
-    xstrlcat(tmp, "nvim.", sizeof(tmp));
-    xstrlcat(tmp, user, sizeof(tmp));
+    if (!after_pathsep(tmp, tmp + tmplen)) {
+      tmplen += (size_t)vim_snprintf(tmp + tmplen, sizeof(tmp) - tmplen, PATHSEPSTR);
+      assert(tmplen < sizeof(tmp));
+    }
+    tmplen += (size_t)vim_snprintf(tmp + tmplen, sizeof(tmp) - tmplen,
+                                   "nvim.%s", user);
+    assert(tmplen < sizeof(tmp));
     os_mkdir(tmp, 0700);  // Always create, to avoid a race.
     bool owned = os_file_owned(tmp);
     bool isdir = os_isdir(tmp);
@@ -3301,7 +3306,10 @@ static void vim_mktempdir(void)
     bool valid = isdir && owned;  // TODO(justinmk): Windows ACL?
 #endif
     if (valid) {
-      add_pathsep(tmp);
+      if (!after_pathsep(tmp, tmp + tmplen)) {
+        tmplen += (size_t)vim_snprintf(tmp + tmplen, sizeof(tmp) - tmplen, PATHSEPSTR);
+        assert(tmplen < sizeof(tmp));
+      }
     } else {
       if (!owned) {
         ELOG("tempdir root not owned by current user (%s): %s", user, tmp);
@@ -3315,11 +3323,15 @@ static void vim_mktempdir(void)
 #endif
       // If our "root" tempdir is invalid or fails, proceed without "<user>/".
       // Else user1 could break user2 by creating "/tmp/nvim.user2/".
-      tmp[strlen(tmp) - strlen(user)] = NUL;
+      tmplen -= strlen(user);
+      tmp[tmplen] = NUL;
     }
 
     // Now try to create "/tmp/nvim.<user>/XXXXXX".
-    xstrlcat(tmp, "XXXXXX", sizeof(tmp));  // mkdtemp "template", will be replaced with random alphanumeric chars.
+    // "XXXXXX" is mkdtemp "template", will be replaced with random alphanumeric chars.
+    tmplen += (size_t)vim_snprintf(tmp + tmplen, sizeof(tmp) - tmplen, "XXXXXX");
+    assert(tmplen < sizeof(tmp));
+    (void)tmplen;
     int r = os_mkdtemp(tmp, path);
     if (r != 0) {
       WLOG("tempdir create failed: %s: %s", os_strerror(r), tmp);

src/nvim/mark.c

@@ -722,10 +722,8 @@ static void fname2fnum(xfmark_T *fm)
 #else
   if (fm->fname[0] == '~' && (fm->fname[1] == '/')) {
 #endif
-
-    expand_env("~/", NameBuff, MAXPATHL);
-    int len = (int)strlen(NameBuff);
-    xstrlcpy(NameBuff + len, fm->fname + 2, (size_t)(MAXPATHL - len));
+    size_t len = expand_env("~/", NameBuff, MAXPATHL);
+    xstrlcpy(NameBuff + len, fm->fname + 2, MAXPATHL - len);
   } else {
     xstrlcpy(NameBuff, fm->fname, MAXPATHL);
   }

src/nvim/os/env.c

@@ -577,9 +577,9 @@ char *expand_env_save_opt(char *src, bool one)
 /// @param src        Input string e.g. "$HOME/vim.hlp"
 /// @param dst[out]   Where to put the result
 /// @param dstlen     Maximum length of the result
-void expand_env(char *src, char *dst, int dstlen)
+size_t expand_env(char *src, char *dst, int dstlen)
 {
-  expand_env_esc(src, dst, dstlen, false, false, NULL);
+  return expand_env_esc(src, dst, dstlen, false, false, NULL);
 }
 
 /// Expand environment variable with path name and escaping.
@@ -591,8 +591,8 @@ void expand_env(char *src, char *dst, int dstlen)
 /// @param esc        Escape spaces in expanded variables
 /// @param one        `srcp` is a single filename
 /// @param prefix     Start again after this (can be NULL)
-void expand_env_esc(const char *restrict srcp, char *restrict dst, int dstlen, bool esc, bool one,
-                    char *prefix)
+size_t expand_env_esc(const char *restrict srcp, char *restrict dst, int dstlen, bool esc, bool one,
+                      char *prefix)
   FUNC_ATTR_NONNULL_ARG(1, 2)
 {
   char *tail;
@@ -600,6 +600,7 @@ void expand_env_esc(const char *restrict srcp, char *restrict dst, int dstlen, b
   bool copy_char;
   bool mustfree;  // var was allocated, need to free it later
   bool at_start = true;  // at start of a name
+  char *const dst_start = dst;
 
   int prefix_len = (prefix == NULL) ? 0 : (int)strlen(prefix);
 
@@ -731,23 +732,24 @@ void expand_env_esc(const char *restrict srcp, char *restrict dst, int dstlen, b
         mustfree = true;
       }
 
-      if (var != NULL && *var != NUL
-          && (strlen(var) + strlen(tail) + 1 < (unsigned)dstlen)) {
-        STRCPY(dst, var);
-        dstlen -= (int)strlen(var);
+      if (var != NULL && *var != NUL) {
         int c = (int)strlen(var);
-        // if var[] ends in a path separator and tail[] starts
-        // with it, skip a character
-        if (after_pathsep(dst, dst + c)
+        if ((size_t)c + strlen(tail) + 1 < (unsigned)dstlen) {
+          STRCPY(dst, var);
+          dstlen -= c;
+          // if var[] ends in a path separator and tail[] starts
+          // with it, skip a character
+          if (after_pathsep(dst, dst + c)
 #if defined(BACKSLASH_IN_FILENAME)
-            && dst[c - 1] != ':'
+              && dst[c - 1] != ':'
 #endif
-            && vim_ispathsep(*tail)) {
-          tail++;
+              && vim_ispathsep(*tail)) {
+            tail++;
+          }
+          dst += c;
+          src = tail;
+          copy_char = false;
         }
-        dst += c;
-        src = tail;
-        copy_char = false;
       }
       if (mustfree) {
         xfree(var);
@@ -778,6 +780,8 @@ void expand_env_esc(const char *restrict srcp, char *restrict dst, int dstlen, b
     }
   }
   *dst = NUL;
+
+  return (size_t)(dst - dst_start);
 }
 
 /// Check if the directory "vimdir/<version>" or "vimdir/runtime" exists.

src/nvim/shada.c

@@ -1314,8 +1314,9 @@ static char *shada_filename(const char *file)
       //     because various expansions must have already be done by the shell.
       //     If shell is not performing them then they should be done in main.c
       //     where arguments are parsed, *not here*.
-      expand_env((char *)file, &(NameBuff[0]), MAXPATHL);
+      size_t len = expand_env((char *)file, &(NameBuff[0]), MAXPATHL);
       file = &(NameBuff[0]);
+      return xmemdupz(file, len);
     }
   }
   return xstrdup(file);