strbuf: Invalidate (only) when stolen

The main for strbuf_steal() to free() on error was to allow the caller
to pass the NULL up the stack with just a return call to
strbuf_steal().

However this is error-prone and surprising that the buffer is still
invalidated on error. Provide an strbuf cleanup attribute that can be
used for the same purpose and make sure that when the string is stolen,
it's set to NULL, so there's no dangling pointer around.

Since we run the  testsuite with AddressSanitizer, a simple test can be
added to make sure the stolen string becomes valid when the attribute is
used.

Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Link: https://github.com/kmod-project/kmod/pull/239

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 1324eeb0dd9439a2b9f2ca61c5a60e0f9ddd3666..17d75cc09cc291ab873d0e95c6d5f27b9877466d 100644 (file)
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -1796,7 +1796,7 @@ static struct kmod_list *kmod_module_info_append(struct kmod_list **list, const
 static char *kmod_module_hex_to_str(const char *hex, size_t len)
 {
        static const char digits[] = "0123456789ABCDEF";
-       struct strbuf sbuf;
+       _cleanup_strbuf_ struct strbuf sbuf;
        const size_t line_limit = 20;
 
        strbuf_init(&sbuf);
@@ -1804,20 +1804,19 @@ static char *kmod_module_hex_to_str(const char *hex, size_t len)
        for (size_t i = 0; i < len; i++) {
                if (!strbuf_pushchar(&sbuf, digits[(hex[i] >> 4) & 0x0F]) ||
                    !strbuf_pushchar(&sbuf, digits[hex[i] & 0x0F]))
-                       goto fail;
+                       return NULL;
+
                if (i < len - 1) {
                        if (!strbuf_pushchar(&sbuf, ':'))
-                               goto fail;
+                               return NULL;
 
                        if ((i + 1) % line_limit == 0 &&
                            !strbuf_pushchars(&sbuf, "\n\t\t"))
-                               goto fail;
+                               return NULL;
                }
        }
+
        return strbuf_steal(&sbuf);
-fail:
-       strbuf_release(&sbuf);
-       return NULL;
 }
 
 static struct kmod_list *kmod_module_info_append_hex(struct kmod_list **list,

diff --git a/shared/strbuf.c b/shared/strbuf.c
index e193d21908568215b5a090c15c8966583820e110..f3a54e23443dac56bfe0883d8a8eb7e0ca5715d2 100644 (file)
--- a/shared/strbuf.c
+++ b/shared/strbuf.c
@@ -55,12 +55,11 @@ char *strbuf_steal(struct strbuf *buf)
 {
        char *bytes;
 
-       if (!buf_realloc(buf, buf->used + 1)) {
-               free(buf->bytes);
+       if (!buf_realloc(buf, buf->used + 1))
                return NULL;
-       }
 
        bytes = buf->bytes;
+       buf->bytes = NULL;
        bytes[buf->used] = '\0';
 
        return bytes;

diff --git a/shared/strbuf.h b/shared/strbuf.h
index 64a5ba00f823e4b2944a6a1b0b5d6b471d14c8bf..2e005e42e04f1ec930c0f3c5f9b44e28e73e4159 100644 (file)
--- a/shared/strbuf.h
+++ b/shared/strbuf.h
@@ -3,6 +3,8 @@
 #include <stdbool.h>
 #include <stddef.h>
 
+#include "macro.h"
+
 /*
  * Buffer abstract data type
  */
@@ -13,10 +15,22 @@ struct strbuf {
 };
 
 void strbuf_init(struct strbuf *buf);
+
 void strbuf_release(struct strbuf *buf);
+#define _cleanup_strbuf_ _cleanup_(strbuf_release)
+
 void strbuf_clear(struct strbuf *buf);
 
-/* Destroy buffer and return a copy as a C string */
+/*
+ * Return a copy as a C string, guaranteed to be nul-terminated. On success, the @buf
+ * becomes invalid and shouldn't be used anymore, except for an (optional) call to
+ * strbuf_release() which still does the right thing on an invalidated buffer. On failure,
+ * NULL is returned and the buffer remains valid: strbuf_release() should be called.
+ * Consider using _cleanup_strbuf_ attribute to release the buffer as needed.
+ *
+ * The copy may use the same underlying storage as the buffer and should be free'd later
+ * with free().
+ */
 char *strbuf_steal(struct strbuf *buf);
 
 /*

diff --git a/testsuite/test-strbuf.c b/testsuite/test-strbuf.c
index e4bfd05892e2923c817ab81f49d0370886083526..9c62349d792d9093b6e5fa6ca5f8c3816869dbfe 100644 (file)
--- a/testsuite/test-strbuf.c
+++ b/testsuite/test-strbuf.c
@@ -87,4 +87,23 @@ static int test_strbuf_pushchars(const struct test *t)
 DEFINE_TEST(test_strbuf_pushchars,
            .description = "test strbuf_{pushchars, popchar, popchars}");
 
+static int test_strbuf_steal(const struct test *t)
+{
+       char *result;
+
+       {
+               _cleanup_strbuf_ struct strbuf buf;
+
+               strbuf_init(&buf);
+               strbuf_pushchars(&buf, TEXT);
+               result = strbuf_steal(&buf);
+       }
+
+       assert_return(streq(result, TEXT), EXIT_FAILURE);
+       free(result);
+
+       return 0;
+}
+DEFINE_TEST(test_strbuf_steal, .description = "test strbuf_steal with cleanup");
+
 TESTSUITE_MAIN();