[libc] Add strncat and fix strcat

[michaelrj]

Summary

This adds strncat to llvm libc. In addition, an error was found with
strcat and that was fixed.

[lntue]

libc/src/string/strncat.cpp

23

I think you should use copyAmount here instead of count, since for strncpy it is specifically said that when srcLength < count, extra '\0' 's will be appended upto count. On the other hand, strncat is only specified to be null-terminated.

libc/test/src/string/strncat_test.cpp

61

Nit: Also add tests where count = srcLength + 1 and count > srcLength + 1.

[michaelrj]

fix strncat writing null bytes beyond the end of src, and add a test for that.

[sivachandra]

libc/src/string/strcat.cpp

22

Its not clear to me as to what this change has done that something got "fixed". A corresponding test would have helped me see it may be.

libc/config/linux/x86_64/entrypoints.txt

set(TARGET_LIBC_ENTRYPOINTS
     libc.src.string.memrchr
     libc.src.string.memset
     libc.src.string.strcat
     libc.src.string.strchr
     libc.src.string.strcmp
     libc.src.string.strcpy
     libc.src.string.strcspn
     libc.src.string.strlen
+    libc.src.string.strncat
     libc.src.string.strncmp
     libc.src.string.strncpy
     libc.src.string.strnlen
     libc.src.string.strpbrk
     libc.src.string.strrchr
     libc.src.string.strspn
     libc.src.string.strstr
     libc.src.string.strtok

libc/src/string/CMakeLists.txt

SRCS
     strlen.cpp
   HDRS
     strlen.h
   DEPENDS
     libc.include.string
 )
 
 add_entrypoint_object(
+  strncat
+  SRCS
+    strncat.cpp
+  HDRS
+    strncat.h
+  DEPENDS
+    .strncpy
+    .string_utils
+)
+
+add_entrypoint_object(
   strncmp
   SRCS
     strncmp.cpp
   HDRS
     strncmp.h
 )
 
 add_entrypoint_object(

libc/src/string/strcat.cpp

 #include "src/string/string_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(char *, strcat,
                    (char *__restrict dest, const char *__restrict src)) {
-  __llvm_libc::strcpy(dest + internal::string_length(dest), src);
+  size_t destLength = internal::string_length(dest);
+  size_t srcLength = internal::string_length(src);
+  __llvm_libc::strcpy(dest + destLength, src);
+  dest[destLength + srcLength] = '\0';
   return dest;
 }
 
 } // namespace __llvm_libc

libc/src/string/strncat.h

+//===-- Implementation header for strncat -----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STRING_STRNCAT_H
+#define LLVM_LIBC_SRC_STRING_STRNCAT_H
+
+#include <string.h>
+
+namespace __llvm_libc {
+
+char *strncat(char *__restrict dest, const char *__restrict src, size_t count);
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STRING_STRNCAT_H

libc/src/string/strncat.cpp

-//===-- Implementation of strcat ------------------------------------------===//
+//===-- Implementation of strncat -----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#include "src/string/strcat.h"
-#include "src/string/strcpy.h"
+#include "src/string/strncat.h"
 #include "src/string/string_utils.h"
+#include "src/string/strncpy.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
-LLVM_LIBC_FUNCTION(char *, strcat,
-                   (char *__restrict dest, const char *__restrict src)) {
-  __llvm_libc::strcpy(dest + internal::string_length(dest), src);
+LLVM_LIBC_FUNCTION(char *, strncat,
+                   (char *__restrict dest, const char *__restrict src,
+                    size_t count)) {
+  size_t srcLength = internal::string_length(src);
+  size_t copyAmount = srcLength > count ? count : srcLength;
+  size_t destLength = internal::string_length(dest);
+  __llvm_libc::strncpy(dest + destLength, src, copyAmount);
+  dest[destLength + copyAmount] = '\0';
   return dest;
 }
 
 } // namespace __llvm_libc

libc/test/src/string/CMakeLists.txt

SUITE
     libc_string_unittests
   SRCS
     strlen_test.cpp
   DEPENDS
     libc.src.string.strlen
 )
 
 add_libc_unittest(
+  strncat_test
+  SUITE
+    libc_string_unittests
+  SRCS
+    strncat_test.cpp
+  DEPENDS
+    libc.src.string.strncat
+)
+
+add_libc_unittest(
   strncmp_test
   SUITE
     libc_string_unittests
   SRCS
     strncmp_test.cpp
   DEPENDS
     libc.src.string.strncmp
 )

libc/test/src/string/strncat_test.cpp

+//===-- Unittests for strncat ---------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/string/strncat.h"
+#include "utils/UnitTest/Test.h"
+
+TEST(LlvmLibcStrNCatTest, EmptyDest) {
+  const char *abc = "abc";
+  char dest[4];
+
+  dest[0] = '\0';
+
+  // Start by copying nothing
+  char *result = __llvm_libc::strncat(dest, abc, 0);
+  ASSERT_EQ(dest, result);
+  ASSERT_EQ(dest[0], '\0');
+
+  // Then copy part of it.
+  result = __llvm_libc::strncat(dest, abc, 1);
+  ASSERT_EQ(dest, result);
+  ASSERT_STREQ(dest, "a");
+
+  // Reset for the last test.
+  dest[0] = '\0';
+
+  // Then copy all of it.
+  result = __llvm_libc::strncat(dest, abc, 3);
+  ASSERT_EQ(dest, result);
+  ASSERT_STREQ(dest, result);
+  ASSERT_STREQ(dest, abc);
+}
+
+TEST(LlvmLibcStrNCatTest, NonEmptyDest) {
+  const char *abc = "abc";
+  char dest[7];
+
+  dest[0] = 'x';
+  dest[1] = 'y';
+  dest[2] = 'z';
+  dest[3] = '\0';
+
+  // Copy only part of the string onto the end
+  char *result = __llvm_libc::strncat(dest, abc, 1);
+  ASSERT_EQ(dest, result);
+  ASSERT_STREQ(dest, "xyza");
+
+  // Copy a bit more, but without resetting.
+  result = __llvm_libc::strncat(dest, abc, 2);
+  ASSERT_EQ(dest, result);
+  ASSERT_STREQ(dest, "xyzaab");
+
+  // Set just the end marker, to make sure it overwrites properly.
+  dest[3] = '\0';
+
+  result = __llvm_libc::strncat(dest, abc, 3);
+  ASSERT_EQ(dest, result);
+  ASSERT_STREQ(dest, "xyzabc");
+
+  // Check that copying still works when count > src length
+  dest[0] = '\0';
+  // And that it doesn't write beyond what is necessary.
+  dest[4] = 'Z';
+  result = __llvm_libc::strncat(dest, abc, 4);
+  ASSERT_EQ(dest, result);
+  ASSERT_STREQ(dest, "abc");
+  ASSERT_EQ(dest[4], 'Z');
+
+  result = __llvm_libc::strncat(dest, abc, 5);
+  ASSERT_EQ(dest, result);
+  ASSERT_STREQ(dest, "abcabc");
+}