From: Florian Forster <octo@collectd.org>
Date: Fri, 12 Jan 2024 16:51:07 +0000 (+0100)
Subject: common: Overhaul the `sstrncpy` implementation.
X-Git-Tag: 6.0.0-rc0~13^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F4233%2Fhead;p=thirdparty%2Fcollectd.git

common: Overhaul the `sstrncpy` implementation.

Properly check all arguments and behave in a sane manner, i.e. don't crash.

I went back and forth a few times on whether to return `NULL` or `dest` when `n == 0`.

*   On the one hand, `n == 0` is not really an error and a situation that could
    naturally occur, e.g. when you're implementing code that appends to the end
    of a string.
*   On the other hand, if we return `NULL` when `n` is zero we can guarantee
    that we will either return `NULL` or a null terminated string.

Ultimately I decided to go with the stronger guarantee, i.e.

```c
if (n == 0) {
  return NULL;
}
```
---

diff --git a/src/utils/common/common.c b/src/utils/common/common.c
index 31e6146af..afa49d4db 100644
--- a/src/utils/common/common.c
+++ b/src/utils/common/common.c
@@ -85,11 +85,13 @@ static pthread_mutex_t strerror_r_lock = PTHREAD_MUTEX_INITIALIZER;
 #endif
 
 char *sstrncpy(char *dest, const char *src, size_t n) {
-  if (n > 0) {
-    strncpy(dest, src, n - 1);
-    dest[n - 1] = 0;
+  if (dest == NULL || n == 0) {
+    return NULL;
   }
 
+  strncpy(dest, src != NULL ? src : "", n - 1);
+  dest[n - 1] = 0;
+
   return dest;
 } /* char *sstrncpy */
 
diff --git a/src/utils/common/common.h b/src/utils/common/common.h
index 1f2194906..069e1b679 100644
--- a/src/utils/common/common.h
+++ b/src/utils/common/common.h
@@ -64,6 +64,13 @@ struct value_to_rate_state_s {
 };
 typedef struct value_to_rate_state_s value_to_rate_state_t;
 
+/* sstrncpy is a safe alternative to strncpy(3). It differs from strncpy(3) in
+ * the following ways:
+ * - If dest is NULL or n is zero, NULL is returned and no writes will take
+ *   place.
+ * - If src is NULL it behaves as if an empty string ("") was provided.
+ * - A null byte is written to dest[n-1] unconditionally. That means the return
+ *   value is either NULL or a null terminated string. */
 char *sstrncpy(char *dest, const char *src, size_t n);
 
 __attribute__((format(printf, 3, 4))) int ssnprintf(char *str, size_t size,
diff --git a/src/utils/common/common_test.c b/src/utils/common/common_test.c
index fabbf2fb8..58a11b2b9 100644
--- a/src/utils/common/common_test.c
+++ b/src/utils/common/common_test.c
@@ -42,28 +42,55 @@ kstat_ctl_t *kc;
 #endif /* HAVE_LIBKSTAT */
 
 DEF_TEST(sstrncpy) {
-  char buffer[16] = "";
-  char *ptr = &buffer[4];
-  char *ret;
-
-  buffer[0] = buffer[1] = buffer[2] = buffer[3] = 0xff;
-  buffer[12] = buffer[13] = buffer[14] = buffer[15] = 0xff;
-
-  ret = sstrncpy(ptr, "foobar", 8);
-  OK(ret == ptr);
-  EXPECT_EQ_STR("foobar", ptr);
-  OK(buffer[3] == buffer[12]);
-
-  ret = sstrncpy(ptr, "abc", 8);
-  OK(ret == ptr);
-  EXPECT_EQ_STR("abc", ptr);
-  OK(buffer[3] == buffer[12]);
-
-  ret = sstrncpy(ptr, "collectd", 8);
-  OK(ret == ptr);
-  OK(ptr[7] == 0);
-  EXPECT_EQ_STR("collect", ptr);
-  OK(buffer[3] == buffer[12]);
+  struct {
+    char const *name;
+    char const *src;
+    size_t size;
+    char const *want;
+  } cases[] = {
+      {
+          .name = "normal copy",
+          .src = "Hello, world!",
+          .size = 16,
+          .want = "Hello, world!",
+      },
+      {
+          .name = "truncated copy",
+          .src = "Hello, world!",
+          .size = 8,
+          .want = "Hello, ",
+      },
+      {
+          .name = "NULL source is treated like an empty string",
+          .src = NULL,
+          .size = 8,
+          .want = "",
+      },
+      {
+          .name = "size is zero",
+          .src = "test",
+          .size = 0,
+          .want = NULL,
+      },
+  };
+
+  for (size_t i = 0; i < STATIC_ARRAY_SIZE(cases); i++) {
+    printf("## Case %zu: %s\n", i + 1, cases[i].name);
+
+    char dest[cases[i].size + 1];
+    memset(dest, 0xff, sizeof(dest));
+
+    char *want_ret = (cases[i].size == 0) ? NULL : dest;
+    EXPECT_EQ_PTR(want_ret, sstrncpy(dest, cases[i].src, cases[i].size));
+    EXPECT_EQ_INT((char)0xff, dest[sizeof(dest) - 1]);
+    if (cases[i].want == NULL) {
+      continue;
+    }
+    EXPECT_EQ_STR(cases[i].want, dest);
+  }
+
+  printf("## Case %zu: dest is NULL\n", STATIC_ARRAY_SIZE(cases) + 1);
+  EXPECT_EQ_PTR(NULL, sstrncpy(NULL, "test", 23));
 
   return 0;
 }