From 25f2b2e09608e4be9bba6cb5c455356c7858b712 Mon Sep 17 00:00:00 2001
From: Lucas De Marchi <lucas.de.marchi@gmail.com>
Date: Tue, 12 Nov 2024 09:34:58 -0600
Subject: [PATCH] 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 | 13 ++++++-------
 shared/strbuf.c          |  5 ++---
 shared/strbuf.h          | 16 +++++++++++++++-
 testsuite/test-strbuf.c  | 19 +++++++++++++++++++
 4 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 1324eeb..17d75cc 100644
--- 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 e193d21..f3a54e2 100644
--- 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 64a5ba0..2e005e4 100644
--- 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 e4bfd05..9c62349 100644
--- 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();