xstrlcat: Allow overlapped pointers. (#6017)

memcpy is not equivalent to memmove (which is used by vim_strcat), this
could cause subtle bugs if xstrlcat is used as a replacement for
vim_strcat. But vim_strcat is inconsistent: in the `else` branch it uses
strcpy, which doesn't allow overlap.

Helped-by: oni-link <knil.ino@gmail.com>
Helped-by: James McCoy <jamessan@jamessan.com>
Helped-by: Nikolai Aleksandrovich Pavlov <kp-pav@yandex.ru>

4 files changed, 83 insertions, 28 deletions

diff --git a/src/clint.py b/src/clint.py
index 4fa78d3c82..ce31822ada 100755
--- a/src/clint.py
+++ b/src/clint.py
@@ -3176,7 +3176,7 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension,
     if match:
         error(filename, linenum, 'runtime/printf', 4,
               'Use xstrlcpy or snprintf instead of %s' % match.group(1))
-    match = Search(r'\b(STRNCAT|strncat|strcat)\b', line)
+    match = Search(r'\b(STRNCAT|strncat|strcat|vim_strcat)\b', line)
     if match:
         error(filename, linenum, 'runtime/printf', 4,
               'Use xstrlcat or snprintf instead of %s' % match.group(1))
diff --git a/src/nvim/memory.c b/src/nvim/memory.c
index 99d4acf25a..25fa2f150e 100644
--- a/src/nvim/memory.c
+++ b/src/nvim/memory.c
@@ -372,15 +372,15 @@ char *xstpncpy(char *restrict dst, const char *restrict src, size_t maxlen)
 ///
 /// @param dst    Buffer to store the result
 /// @param src    String to be copied
-/// @param size   Size of `dst`
+/// @param dsize  Size of `dst`
 /// @return       strlen(src). If retval >= dstsize, truncation occurs.
-size_t xstrlcpy(char *restrict dst, const char *restrict src, size_t size)
+size_t xstrlcpy(char *restrict dst, const char *restrict src, size_t dsize)
   FUNC_ATTR_NONNULL_ALL
 {
   size_t slen = strlen(src);
 
-  if (size) {
-    size_t len = MIN(slen, size - 1);
+  if (dsize) {
+    size_t len = MIN(slen, dsize - 1);
     memcpy(dst, src, len);
     dst[len] = '\0';
   }
@@ -388,31 +388,34 @@ size_t xstrlcpy(char *restrict dst, const char *restrict src, size_t size)
   return slen;  // Does not include NUL.
 }
 
-/// xstrlcat - Appends string src to the end of dst.
+/// Appends `src` to string `dst` of size `dsize` (unlike strncat, dsize is the
+/// full size of `dst`, not space left).  At most dsize-1 characters
+/// will be copied.  Always NUL terminates. `src` and `dst` may overlap.
 ///
-/// Compatible with *BSD strlcat: Appends at most (dstsize - strlen(dst) - 1)
-/// characters. dst will be NUL-terminated.
+/// @see vim_strcat from Vim.
+/// @see strlcat from OpenBSD.
 ///
-/// Note: Replaces `vim_strcat`.
-///
-/// @param dst      Buffer to store the string
-/// @param src      String to be copied
-/// @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)
+/// @param dst      Buffer to be appended-to. Must have a NUL byte.
+/// @param src      String to put at the end of `dst`
+/// @param dsize    Size of `dst` including NUL byte. Must be greater than 0.
+/// @return         strlen(src) + strlen(initial dst)
+///                 If retval >= dsize, truncation occurs.
+size_t xstrlcat(char *const dst, const char *const src, const size_t dsize)
   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';
+  assert(dsize > 0);
+  const size_t dlen = strlen(dst);
+  assert(dlen < dsize);
+  const size_t slen = strlen(src);
+
+  if (slen > dsize - dlen - 1) {
+    memmove(dst + dlen, src, dsize - dlen - 1);
+    dst[dsize - 1] = '\0';
+  } else {
+    memmove(dst + dlen, src, slen + 1);
   }
-  return ret;  // Does not include NUL.
+
+  return slen + dlen;  // Does not include NUL.
 }
 
 /// strdup() wrapper
diff --git a/src/nvim/window.c b/src/nvim/window.c
index 89228e1b0f..801969a594 100644
--- a/src/nvim/window.c
+++ b/src/nvim/window.c
@@ -129,9 +129,10 @@ newwindow:
       vim_snprintf(cbuf, sizeof(cbuf) - 5, "%" PRId64, (int64_t)Prenum);
     else
       cbuf[0] = NUL;
-    if (nchar == 'v' || nchar == Ctrl_V)
-      strcat(cbuf, "v");
-    strcat(cbuf, "new");
+    if (nchar == 'v' || nchar == Ctrl_V) {
+      xstrlcat(cbuf, "v", sizeof(cbuf));
+    }
+    xstrlcat(cbuf, "new", sizeof(cbuf));
     do_cmdline_cmd(cbuf);
     break;
 
diff --git a/test/unit/memory_spec.lua b/test/unit/memory_spec.lua
new file mode 100644
index 0000000000..73a32724ef
--- /dev/null
+++ b/test/unit/memory_spec.lua
@@ -0,0 +1,51 @@
+local helpers = require("test.unit.helpers")
+
+local cimport = helpers.cimport
+local cstr = helpers.cstr
+local eq = helpers.eq
+local ffi = helpers.ffi
+local to_cstr = helpers.to_cstr
+
+local cimp = cimport('stdlib.h', './src/nvim/memory.h')
+
+describe('xstrlcat()', function()
+  local function test_xstrlcat(dst, src, dsize)
+    assert.is_true(dsize >= 1 + string.len(dst))  -- sanity check for tests
+    local dst_cstr = cstr(dsize, dst)
+    local src_cstr = to_cstr(src)
+    eq(string.len(dst .. src), cimp.xstrlcat(dst_cstr, src_cstr, dsize))
+    return ffi.string(dst_cstr)
+  end
+
+  local function test_xstrlcat_overlap(dst, src_idx, dsize)
+    assert.is_true(dsize >= 1 + string.len(dst))  -- sanity check for tests
+    local dst_cstr = cstr(dsize, dst)
+    local src_cstr = dst_cstr + src_idx  -- pointer into `dst` (overlaps)
+    eq(string.len(dst) + string.len(dst) - src_idx,
+       cimp.xstrlcat(dst_cstr, src_cstr, dsize))
+    return ffi.string(dst_cstr)
+  end
+
+  it('concatenates strings', function()
+    eq('ab', test_xstrlcat('a', 'b', 3))
+    eq('ab', test_xstrlcat('a', 'b', 4096))
+    eq('ABCיהZdefgiיהZ',  test_xstrlcat('ABCיהZ', 'defgiיהZ',  4096))
+    eq('b',  test_xstrlcat('',  'b', 4096))
+    eq('a',  test_xstrlcat('a', '',  4096))
+  end)
+
+  it('concatenates overlapping strings', function()
+    eq('abcabc',  test_xstrlcat_overlap('abc', 0, 7))
+    eq('abca',    test_xstrlcat_overlap('abc', 0, 5))
+    eq('abcb',    test_xstrlcat_overlap('abc', 1, 5))
+    eq('abcc',    test_xstrlcat_overlap('abc', 2, 10))
+    eq('abcabc',  test_xstrlcat_overlap('abc', 0, 2343))
+  end)
+
+  it('truncates if `dsize` is too small', function()
+    eq('a', test_xstrlcat('a', 'b', 2))
+    eq('', test_xstrlcat('', 'b', 1))
+    eq('ABCיהZd',  test_xstrlcat('ABCיהZ', 'defgiיהZ',  10))
+  end)
+
+end)