[compiler-rt] Add internal implementations for strlcat and strlcpy

[zaks.anna]

Summary

The compiler-rt should make use of strlcpy() rather than strncpy(). Using internal_strncpy() may be fine with appropriate bounds checking or enforcement of nul-termination elsewhere, but it's just good practice these days to avoid using strncpy() in new code.

A patch by Jeremy Sequoia!

[kcc]

Please clang-format and add tests (to test/sanitizer_common/TestCases)

[kcc]

I mean, clang-format only the new parts of code.

[samsonov]

Um, I think we need unit tests for this, they should go to lib/sanitizer_common/tests

Do you plan to actually replace internal_strncpy / internal_strncat in the subsequent changes?

[zaks.anna]

Added tests and re-formatted.

[zaks.anna]

Do you plan to actually replace internal_strncpy / internal_strncat in the subsequent changes?

No. I want to use it in an upcoming patch. Do you think we should replace all uses?

[glider]

lib/sanitizer_common/sanitizer_libc.cc

181

Nit: missing spaces around '+'

185

Ditto (also '-')

203

dst and src must not overlap, but maybe we use memmove() here just to make sure nothing blows up?

[zaks.anna]

Addressed review by glider.

[kcc]

LGTM with one nit

lib/sanitizer_common/tests/sanitizer_libc_test.cc

149

use

static const size_t kStrlcpyBufSize = 8;

lib/sanitizer_common/sanitizer_libc.h

 void *internal_memset(void *s, int c, uptr n);
 char* internal_strchr(const char *s, int c);
 char *internal_strchrnul(const char *s, int c);
 int internal_strcmp(const char *s1, const char *s2);
 uptr internal_strcspn(const char *s, const char *reject);
 char *internal_strdup(const char *s);
 char *internal_strndup(const char *s, uptr n);
 uptr internal_strlen(const char *s);
+uptr internal_strlcat(char *dst, const char *src, uptr maxlen);
 char *internal_strncat(char *dst, const char *src, uptr n);
 int internal_strncmp(const char *s1, const char *s2, uptr n);
+uptr internal_strlcpy(char *dst, const char *src, uptr maxlen);
 char *internal_strncpy(char *dst, const char *src, uptr n);
 uptr internal_strnlen(const char *s, uptr maxlen);
 char *internal_strrchr(const char *s, int c);
 // This is O(N^2), but we are not using it in hot places.
 char *internal_strstr(const char *haystack, const char *needle);
 // Works only for base=10 and doesn't set errno.
 s64 internal_simple_strtoll(const char *nptr, char **endptr, int base);
 int internal_snprintf(char *buffer, uptr length, const char *format, ...);

lib/sanitizer_common/sanitizer_libc.cc

 }
 
 uptr internal_strlen(const char *s) {
   uptr i = 0;
   while (s[i]) i++;
   return i;
 }
 
+uptr internal_strlcat(char *dst, const char *src, uptr maxlen) {
+  const uptr srclen = internal_strlen(src);
+  const uptr dstlen = internal_strnlen(dst, maxlen);
+  if (dstlen == maxlen) return maxlen + srclen;
+  if (srclen < maxlen - dstlen) {
+    internal_memmove(dst + dstlen, src, srclen + 1);
+  } else {
+    internal_memmove(dst + dstlen, src, maxlen - dstlen - 1);
+    dst[maxlen - 1] = '\0';
+  }
+  return dstlen + srclen;
+}
+
 char *internal_strncat(char *dst, const char *src, uptr n) {
   uptr len = internal_strlen(dst);
   uptr i;
   for (i = 0; i < n && src[i]; i++)
     dst[len + i] = src[i];
   dst[len + i] = 0;
   return dst;
 }
 
+uptr internal_strlcpy(char *dst, const char *src, uptr maxlen) {
+  const uptr srclen = internal_strlen(src);
+  if (srclen < maxlen) {
+    internal_memmove(dst, src, srclen + 1);
+  } else if (maxlen != 0) {
+    internal_memmove(dst, src, maxlen - 1);
+    dst[maxlen - 1] = '\0';
+  }
+  return srclen;
+}
+
 char *internal_strncpy(char *dst, const char *src, uptr n) {
   uptr i;
   for (i = 0; i < n && src[i]; i++)
     dst[i] = src[i];
   internal_memset(dst + i, '\0', n - i);
   return dst;
 }
 

lib/sanitizer_common/tests/sanitizer_libc_test.cc

 //===-- sanitizer_libc_test.cc --------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 // Tests for sanitizer_libc.h.
 //===----------------------------------------------------------------------===//
+#include <algorithm>
 
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_platform.h"
 #include "gtest/gtest.h"
 
 #if SANITIZER_WINDOWS
 #include <windows.h>
#if SANITIZER_WINDOWS
   // No sanitizer needs to delete a file on Windows yet. If we ever do, we can
   // add a portable wrapper and test it from here.
   ::DeleteFileA(&tmpfile[0]);
 #else
   internal_unlink(tmpfile);
 #endif
 }
 
+#define STRLCPY_BUF_SIZE 8
+void test_internal_strlcpy(char *dbuf, const char *sbuf) {
+  uptr retval = 0;
+  retval = internal_strlcpy(dbuf, sbuf, STRLCPY_BUF_SIZE);
+  EXPECT_EQ(internal_strncmp(dbuf, sbuf, STRLCPY_BUF_SIZE - 1), 0);
+  EXPECT_EQ(internal_strlen(dbuf),
+            std::min(internal_strlen(sbuf), (uptr)(STRLCPY_BUF_SIZE - 1)));
+  EXPECT_EQ(retval, internal_strlen(sbuf));
+
+  // Test with shorter maxlen.
+  uptr maxlen = 2;
+  if (internal_strlen(sbuf) > maxlen) {
+    retval = internal_strlcpy(dbuf, sbuf, maxlen);
+    EXPECT_EQ(internal_strncmp(dbuf, sbuf, maxlen - 1), 0);
+    EXPECT_EQ(internal_strlen(dbuf), maxlen - 1);
+  }
+}
+
 TEST(SanitizerCommon, InternalStrFunctions) {
   const char *haystack = "haystack";
   EXPECT_EQ(haystack + 2, internal_strchr(haystack, 'y'));
   EXPECT_EQ(haystack + 2, internal_strchrnul(haystack, 'y'));
   EXPECT_EQ(0, internal_strchr(haystack, 'z'));
   EXPECT_EQ(haystack + 8, internal_strchrnul(haystack, 'z'));
+
+  char dbuf[STRLCPY_BUF_SIZE] = {};
+  const char *samesizestr = "1234567";
+  const char *shortstr = "123";
+  const char *longerstr = "123456789";
+
+  // Test internal_strlcpy.
+  internal_strlcpy(dbuf, shortstr, 0);
+  EXPECT_EQ(dbuf[0], 0);
+  EXPECT_EQ(dbuf[0], 0);
+  test_internal_strlcpy(dbuf, samesizestr);
+  test_internal_strlcpy(dbuf, shortstr);
+  test_internal_strlcpy(dbuf, longerstr);
+
+  // Test internal_strlcat.
+  char dcatbuf[STRLCPY_BUF_SIZE] = {};
+  uptr retval = 0;
+  retval = internal_strlcat(dcatbuf, "aaa", 0);
+  EXPECT_EQ(internal_strlen(dcatbuf), (uptr)0);
+  EXPECT_EQ(retval, (uptr)3);
+
+  retval = internal_strlcat(dcatbuf, "123", STRLCPY_BUF_SIZE);
+  EXPECT_EQ(internal_strcmp(dcatbuf, "123"), 0);
+  EXPECT_EQ(internal_strlen(dcatbuf), (uptr)3);
+  EXPECT_EQ(retval, (uptr)3);
+
+  retval = internal_strlcat(dcatbuf, "123", STRLCPY_BUF_SIZE);
+  EXPECT_EQ(internal_strcmp(dcatbuf, "123123"), 0);
+  EXPECT_EQ(internal_strlen(dcatbuf), (uptr)6);
+  EXPECT_EQ(retval, (uptr)6);
+
+  retval = internal_strlcat(dcatbuf, "123", STRLCPY_BUF_SIZE);
+  EXPECT_EQ(internal_strcmp(dcatbuf, "1231231"), 0);
+  EXPECT_EQ(internal_strlen(dcatbuf), (uptr)7);
+  EXPECT_EQ(retval, (uptr)9);
 }
 
 // FIXME: File manipulations are not yet supported on Windows
 #if SANITIZER_POSIX && !SANITIZER_MAC
 TEST(SanitizerCommon, InternalMmapWithOffset) {
   char tmpfile[128];
   temp_file_name(tmpfile, sizeof(tmpfile),
                  "sanitizer_common.internalmmapwithoffset.tmp.");