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

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,

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;

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);
 
 /*

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();