From fb8dba413f2bcaa61c15d1854b28112e3e91a035 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Thu, 19 Jun 2025 10:21:33 +0800 Subject: [PATCH] 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 https://github.com/vim/vim/commit/fff0132399082b7d012d0c1291cf0c6c99e200ff Co-authored-by: John Marriott --- src/nvim/file_search.c | 3 +-- src/nvim/fileio.c | 26 +++++++++++++++++++------- src/nvim/mark.c | 6 ++---- src/nvim/os/env.c | 38 +++++++++++++++++++++----------------- src/nvim/shada.c | 3 ++- 5 files changed, 45 insertions(+), 31 deletions(-) diff --git a/src/nvim/file_search.c b/src/nvim/file_search.c index b0029e7761..c882777f1c 100644 --- a/src/nvim/file_search.c +++ b/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 " ". diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c index 55e6fb1442..c40371d4cf 100644 --- a/src/nvim/fileio.c +++ b/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./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./". - 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 "/". // 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./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); diff --git a/src/nvim/mark.c b/src/nvim/mark.c index 485d077f5c..79c0ac3e0b 100644 --- a/src/nvim/mark.c +++ b/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); } diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c index 4acc243a92..037efdbc06 100644 --- a/src/nvim/os/env.c +++ b/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/" or "vimdir/runtime" exists. diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 8ea54a6923..af8e0e4417 100644 --- a/src/nvim/shada.c +++ b/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);