Introduce _cleanup_(strbuf_cleanupp) and use it to fix null deref on error

catalog_update() would call strbuf_cleanup(NULL) on allocation error.
CID #1390928.

diff --git a/src/basic/strbuf.h b/src/basic/strbuf.h
index 9104bc7e933b85d5a74a6e15f915c1abcca2e6d1..84c2e4bfa582221caf904ce8e71c34d22d3ebc77 100644 (file)
--- a/src/basic/strbuf.h
+++ b/src/basic/strbuf.h
@@ -11,6 +11,8 @@
 #include <stdint.h>
 #include <sys/types.h>
 
+#include "macro.h"
+
 struct strbuf {
         char *buf;
         size_t len;
@@ -40,3 +42,4 @@ struct strbuf *strbuf_new(void);
 ssize_t strbuf_add_string(struct strbuf *str, const char *s, size_t len);
 void strbuf_complete(struct strbuf *str);
 void strbuf_cleanup(struct strbuf *str);
+DEFINE_TRIVIAL_CLEANUP_FUNC(struct strbuf*, strbuf_cleanup);

diff --git a/src/journal/catalog.c b/src/journal/catalog.c
index 9c13d7b46d6b879068047962279eccce3c742c4c..24bf427103a39ac233acd470820290c4c4ec7978 100644 (file)
--- a/src/journal/catalog.c
+++ b/src/journal/catalog.c
@@ -448,7 +448,7 @@ error:
 int catalog_update(const char* database, const char* root, const char* const* dirs) {
         _cleanup_strv_free_ char **files = NULL;
         char **f;
-        struct strbuf *sb = NULL;
+        _cleanup_(strbuf_cleanupp) struct strbuf *sb = NULL;
         _cleanup_hashmap_free_free_free_ Hashmap *h = NULL;
         _cleanup_free_ CatalogItem *items = NULL;
         ssize_t offset;
@@ -461,38 +461,29 @@ int catalog_update(const char* database, const char* root, const char* const* di
 
         h = hashmap_new(&catalog_hash_ops);
         sb = strbuf_new();
-
-        if (!h || !sb) {
-                r = log_oom();
-                goto finish;
-        }
+        if (!h || !sb)
+                return log_oom();
 
         r = conf_files_list_strv(&files, ".catalog", root, 0, dirs);
-        if (r < 0) {
-                log_error_errno(r, "Failed to get catalog files: %m");
-                goto finish;
-        }
+        if (r < 0)
+                return log_error_errno(r, "Failed to get catalog files: %m");
 
         STRV_FOREACH(f, files) {
                 log_debug("Reading file '%s'", *f);
                 r = catalog_import_file(h, *f);
-                if (r < 0) {
-                        log_error_errno(r, "Failed to import file '%s': %m", *f);
-                        goto finish;
-                }
+                if (r < 0)
+                        return log_error_errno(r, "Failed to import file '%s': %m", *f);
         }
 
         if (hashmap_size(h) <= 0) {
                 log_info("No items in catalog.");
-                goto finish;
+                return 0;
         } else
                 log_debug("Found %u items in catalog.", hashmap_size(h));
 
         items = new(CatalogItem, hashmap_size(h));
-        if (!items) {
-                r = log_oom();
-                goto finish;
-        }
+        if (!items)
+                return log_oom();
 
         n = 0;
         HASHMAP_FOREACH_KEY(payload, i, h, j) {
@@ -501,10 +492,9 @@ int catalog_update(const char* database, const char* root, const char* const* di
                           isempty(i->language) ? "C" : i->language);
 
                 offset = strbuf_add_string(sb, payload, strlen(payload));
-                if (offset < 0) {
-                        r = log_oom();
-                        goto finish;
-                }
+                if (offset < 0)
+                        return log_oom();
+
                 i->offset = htole64((uint64_t) offset);
                 items[n++] = *i;
         }
@@ -516,17 +506,11 @@ int catalog_update(const char* database, const char* root, const char* const* di
 
         sz = write_catalog(database, sb, items, n);
         if (sz < 0)
-                r = log_error_errno(sz, "Failed to write %s: %m", database);
-        else {
-                r = 0;
-                log_debug("%s: wrote %u items, with %zu bytes of strings, %"PRIi64" total size.",
-                          database, n, sb->len, sz);
-        }
-
-finish:
-        strbuf_cleanup(sb);
+                return log_error_errno(sz, "Failed to write %s: %m", database);
 
-        return r;
+        log_debug("%s: wrote %u items, with %zu bytes of strings, %"PRIi64" total size.",
+                  database, n, sb->len, sz);
+        return 0;
 }
 
 static int open_mmap(const char *database, int *_fd, struct stat *_st, void **_p) {

diff --git a/src/journal/test-catalog.c b/src/journal/test-catalog.c
index 9e3476d405e617f0841e2627aef990cdf89e6a4f..d5640c549cf6a68be29c0f5721e5e4afa03ade1b 100644 (file)
--- a/src/journal/test-catalog.c
+++ b/src/journal/test-catalog.c
@@ -177,16 +177,16 @@ static void test_catalog_update(void) {
 
         /* Test what happens if there are no files. */
         r = catalog_update(database, NULL, NULL);
-        assert_se(r >= 0);
+        assert_se(r == 0);
 
         /* Test what happens if there are no files in the directory. */
         r = catalog_update(database, NULL, no_catalog_dirs);
-        assert_se(r >= 0);
+        assert_se(r == 0);
 
         /* Make sure that we at least have some files loaded or the
            catalog_list below will fail. */
         r = catalog_update(database, NULL, catalog_dirs);
-        assert_se(r >= 0);
+        assert_se(r == 0);
 }
 
 static void test_catalog_file_lang(void) {

diff --git a/src/test/test-strbuf.c b/src/test/test-strbuf.c
index 7fe9d662232084592ec8188b0441d55121eacdd5..8e5b8b1d776ac2dbc43c1d268aa2fc16637cb9c0 100644 (file)
--- a/src/test/test-strbuf.c
+++ b/src/test/test-strbuf.c
@@ -18,7 +18,7 @@ static ssize_t add_string(struct strbuf *sb, const char *s) {
 }
 
 static void test_strbuf(void) {
-        struct strbuf *sb;
+        _cleanup_(strbuf_cleanupp) struct strbuf *sb;
         _cleanup_strv_free_ char **l;
         ssize_t a, b, c, d, e, f, g, h;
 
@@ -72,8 +72,6 @@ static void test_strbuf(void) {
 
         strbuf_complete(sb);
         assert_se(sb->root == NULL);
-
-        strbuf_cleanup(sb);
 }
 
 int main(int argc, const char *argv[]) {