Introduce free_and_strndup and use it in bus-message.c

v2: fix error in free_and_strndup()

When the orignal and copied message were the same, but shorter than specified
length l, memory read past the end of the buffer would be performed. A test
case is included: a string that had an embedded NUL ("q\0") is used to replace
"q".

v3: Fix one more bug in free_and_strndup and add tests.

v4: Some style fixed based on review, one more use of free_and_replace, and
make the tests more comprehensive.

src/basic/string-util.c

@@ -1004,7 +1004,7 @@ int free_and_strdup(char **p, const char *s) {
 
         assert(p);
 
-        /* Replaces a string pointer with an strdup()ed new string,
+        /* Replaces a string pointer with a strdup()ed new string,
          * possibly freeing the old one. */
 
         if (streq_ptr(*p, s))
@@ -1023,6 +1023,32 @@ int free_and_strdup(char **p, const char *s) {
         return 1;
 }
 
+int free_and_strndup(char **p, const char *s, size_t l) {
+        char *t;
+
+        assert(p);
+        assert(s || l == 0);
+
+        /* Replaces a string pointer with a strndup()ed new string,
+         * freeing the old one. */
+
+        if (!*p && !s)
+                return 0;
+
+        if (*p && s && strneq(*p, s, l) && (l > strlen(*p) || (*p)[l] == '\0'))
+                return 0;
+
+        if (s) {
+                t = strndup(s, l);
+                if (!t)
+                        return -ENOMEM;
+        } else
+                t = NULL;
+
+        free_and_replace(*p, t);
+        return 1;
+}
+
 #if !HAVE_EXPLICIT_BZERO
 /*
  * Pointer to memset is volatile so that compiler must de-reference

src/basic/string-util.h

@@ -176,6 +176,7 @@ char *strrep(const char *s, unsigned n);
 int split_pair(const char *s, const char *sep, char **l, char **r);
 
 int free_and_strdup(char **p, const char *s);
+int free_and_strndup(char **p, const char *s, size_t l);
 
 /* Normal memmem() requires haystack to be nonnull, which is annoying for zero-length buffers */
 static inline void *memmem_safe(const void *haystack, size_t haystacklen, const void *needle, size_t needlelen) {

src/libsystemd/sd-bus/bus-message.c

@@ -4144,20 +4144,19 @@ _public_ int sd_bus_message_peek_type(sd_bus_message *m, char *type, const char
 
                 if (contents) {
                         size_t l;
-                        char *sig;
 
                         r = signature_element_length(c->signature+c->index+1, &l);
                         if (r < 0)
                                 return r;
 
-                        assert(l >= 1);
+                        /* signature_element_length does verification internally */
 
-                        sig = strndup(c->signature + c->index + 1, l);
+                        assert(l >= 1);
-                        if (!sig)
+                        if (free_and_strndup(&c->peeked_signature,
+                                             c->signature + c->index + 1, l) < 0)
                                 return -ENOMEM;
 
-                        free(c->peeked_signature);
+                        *contents = c->peeked_signature;
-                        *contents = c->peeked_signature = sig;
                 }
 
                 if (type)
@@ -4170,19 +4169,17 @@ _public_ int sd_bus_message_peek_type(sd_bus_message *m, char *type, const char
 
                 if (contents) {
                         size_t l;
-                        char *sig;
 
                         r = signature_element_length(c->signature+c->index, &l);
                         if (r < 0)
                                 return r;
 
                         assert(l >= 2);
-                        sig = strndup(c->signature + c->index + 1, l - 2);
+                        if (free_and_strndup(&c->peeked_signature,
-                        if (!sig)
+                                             c->signature + c->index + 1, l - 2) < 0)
                                 return -ENOMEM;
 
-                        free(c->peeked_signature);
+                        *contents = c->peeked_signature;
-                        *contents = c->peeked_signature = sig;
                 }
 
                 if (type)
@@ -4222,9 +4219,8 @@ _public_ int sd_bus_message_peek_type(sd_bus_message *m, char *type, const char
                                 if (k > c->item_size)
                                         return -EBADMSG;
 
-                                free(c->peeked_signature);
+                                if (free_and_strndup(&c->peeked_signature,
-                                c->peeked_signature = strndup((char*) q + 1, k - 1);
+                                                     (char*) q + 1, k - 1) < 0)
-                                if (!c->peeked_signature)
                                         return -ENOMEM;
 
                                 if (!signature_is_valid(c->peeked_signature, true))
@@ -5056,25 +5052,21 @@ int bus_message_parse_fields(sd_bus_message *m) {
 
                         if (*p == 0) {
                                 size_t l;
-                                char *c;
 
                                 /* We found the beginning of the signature
                                  * string, yay! We require the body to be a
                                  * structure, so verify it and then strip the
                                  * opening/closing brackets. */
 
-                                l = ((char*) m->footer + m->footer_accessible) - p - (1 + sz);
+                                l = (char*) m->footer + m->footer_accessible - p - (1 + sz);
                                 if (l < 2 ||
                                     p[1] != SD_BUS_TYPE_STRUCT_BEGIN ||
                                     p[1 + l - 1] != SD_BUS_TYPE_STRUCT_END)
                                         return -EBADMSG;
 
-                                c = strndup(p + 1 + 1, l - 2);
+                                if (free_and_strndup(&m->root_container.signature,
-                                if (!c)
+                                                     p + 1 + 1, l - 2) < 0)
                                         return -ENOMEM;
-
-                                free(m->root_container.signature);
-                                m->root_container.signature = c;
                                 break;
                         }
 

src/test/test-string-util.c

@@ -5,6 +5,7 @@
 #include "macro.h"
 #include "string-util.h"
 #include "strv.h"
+#include "tests.h"
 #include "utf8.h"
 
 static void test_string_erase(void) {
@@ -30,6 +31,64 @@ static void test_string_erase(void) {
         assert_se(x[9] == '\0');
 }
 
+static void test_free_and_strndup_one(char **t, const char *src, size_t l, const char *expected, bool change) {
+        int r;
+
+        log_debug("%s: \"%s\", \"%s\", %zd (expect \"%s\", %s)",
+                  __func__, strnull(*t), strnull(src), l, strnull(expected), yes_no(change));
+
+        r = free_and_strndup(t, src, l);
+        assert_se(streq_ptr(*t, expected));
+        assert_se(r == change); /* check that change occurs only when necessary */
+}
+
+static void test_free_and_strndup(void) {
+        static const struct test_case {
+                const char *src;
+                size_t len;
+                const char *expected;
+        } cases[] = {
+                     {"abc", 0, ""},
+                     {"abc", 0, ""},
+                     {"abc", 1, "a"},
+                     {"abc", 2, "ab"},
+                     {"abc", 3, "abc"},
+                     {"abc", 4, "abc"},
+                     {"abc", 5, "abc"},
+                     {"abc", 5, "abc"},
+                     {"abc", 4, "abc"},
+                     {"abc", 3, "abc"},
+                     {"abc", 2, "ab"},
+                     {"abc", 1, "a"},
+                     {"abc", 0, ""},
+
+                     {"", 0, ""},
+                     {"", 1, ""},
+                     {"", 2, ""},
+                     {"", 0, ""},
+                     {"", 1, ""},
+                     {"", 2, ""},
+                     {"", 2, ""},
+                     {"", 1, ""},
+                     {"", 0, ""},
+
+                     {NULL, 0, NULL},
+
+                     {"foo", 3, "foo"},
+                     {"foobar", 6, "foobar"},
+        };
+
+        _cleanup_free_ char *t = NULL;
+        const char *prev_expected = t;
+
+        for (unsigned i = 0; i < ELEMENTSOF(cases); i++) {
+                test_free_and_strndup_one(&t,
+                                          cases[i].src, cases[i].len, cases[i].expected,
+                                          !streq_ptr(cases[i].expected, prev_expected));
+                prev_expected = t;
+        }
+}
+
 static void test_ascii_strcasecmp_n(void) {
 
         assert_se(ascii_strcasecmp_n("", "", 0) == 0);
@@ -520,7 +579,10 @@ static void test_memory_startswith_no_case(void) {
 }
 
 int main(int argc, char *argv[]) {
+        test_setup_logging(LOG_DEBUG);
+
         test_string_erase();
+        test_free_and_strndup();
         test_ascii_strcasecmp_n();
         test_ascii_strcasecmp_nn();
         test_cellescape();