From 9d0d39ee53550f7c5d42f662068f3e336215bbb7 Mon Sep 17 00:00:00 2001
From: Yu Watanabe <watanabe.yu+github@gmail.com>
Date: Mon, 6 Mar 2023 05:56:29 +0900
Subject: [PATCH] conf-files: fix potential memleak in
 conf_files_list_strv_internal() on failure

This also changes the used hash_ops from path_hash_ops to
string_hash_ops, as the key is not a path, but a filename.
---
 src/basic/conf-files.c | 70 +++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/src/basic/conf-files.c b/src/basic/conf-files.c
index 3fbb2cda001..d64277b806b 100644
--- a/src/basic/conf-files.c
+++ b/src/basic/conf-files.c
@@ -23,8 +23,8 @@
 #include "terminal-util.h"
 
 static int files_add(
-                Hashmap *h,
-                Set *masked,
+                Hashmap **h,
+                Set **masked,
                 const char *suffix,
                 const char *root,
                 unsigned flags,
@@ -35,7 +35,7 @@ static int files_add(
         int r;
 
         assert(h);
-        assert((flags & CONF_FILES_FILTER_MASKED) == 0 || masked);
+        assert(masked);
         assert(path);
 
         r = chase_symlinks_and_opendir(path, root, CHASE_PREFIX_ROOT, &dirpath, &dir);
@@ -45,21 +45,21 @@ static int files_add(
                 return log_debug_errno(r, "Failed to open directory '%s/%s': %m", empty_or_root(root) ? "" : root, dirpath);
 
         FOREACH_DIRENT(de, dir, return -errno) {
+                _cleanup_free_ char *n = NULL, *p = NULL;
                 struct stat st;
-                char *p, *key;
 
                 /* Does this match the suffix? */
                 if (suffix && !endswith(de->d_name, suffix))
                         continue;
 
                 /* Has this file already been found in an earlier directory? */
-                if (hashmap_contains(h, de->d_name)) {
+                if (hashmap_contains(*h, de->d_name)) {
                         log_debug("Skipping overridden file '%s/%s'.", dirpath, de->d_name);
                         continue;
                 }
 
                 /* Has this been masked in an earlier directory? */
-                if ((flags & CONF_FILES_FILTER_MASKED) && set_contains(masked, de->d_name)) {
+                if ((flags & CONF_FILES_FILTER_MASKED) && set_contains(*masked, de->d_name)) {
                         log_debug("File '%s/%s' is masked by previous entry.", dirpath, de->d_name);
                         continue;
                 }
@@ -74,10 +74,8 @@ static int files_add(
                 /* Is this a masking entry? */
                 if ((flags & CONF_FILES_FILTER_MASKED))
                         if (null_or_empty(&st)) {
-                                assert(masked);
-
                                 /* Mark this one as masked */
-                                r = set_put_strdup(&masked, de->d_name);
+                                r = set_put_strdup(masked, de->d_name);
                                 if (r < 0)
                                         return r;
 
@@ -104,27 +102,25 @@ static int files_add(
                                 continue;
                         }
 
-                if (flags & CONF_FILES_BASENAME) {
-                        p = strdup(de->d_name);
-                        if (!p)
-                                return -ENOMEM;
+                n = strdup(de->d_name);
+                if (!n)
+                        return -ENOMEM;
 
-                        key = p;
-                } else {
+                if ((flags & CONF_FILES_BASENAME))
+                        r = hashmap_ensure_put(h, &string_hash_ops_free, n, n);
+                else {
                         p = path_join(dirpath, de->d_name);
                         if (!p)
                                 return -ENOMEM;
 
-                        key = basename(p);
+                        r = hashmap_ensure_put(h, &string_hash_ops_free_free, n, p);
                 }
-
-                r = hashmap_put(h, key, p);
-                if (r < 0) {
-                        free(p);
-                        return log_debug_errno(r, "Failed to add item to hashmap: %m");
-                }
-
+                if (r < 0)
+                        return r;
                 assert(r > 0);
+
+                TAKE_PTR(n);
+                TAKE_PTR(p);
         }
 
         return 0;
@@ -142,8 +138,9 @@ static int conf_files_list_strv_internal(
                 char **dirs) {
 
         _cleanup_hashmap_free_ Hashmap *fh = NULL;
-        _cleanup_set_free_free_ Set *masked = NULL;
-        char **files;
+        _cleanup_set_free_ Set *masked = NULL;
+        _cleanup_strv_free_ char **files = NULL;
+        _cleanup_free_ char **sv = NULL;
         int r;
 
         assert(ret);
@@ -152,30 +149,25 @@ static int conf_files_list_strv_internal(
         if (!path_strv_resolve_uniq(dirs, root))
                 return -ENOMEM;
 
-        fh = hashmap_new(&path_hash_ops);
-        if (!fh)
-                return -ENOMEM;
-
-        if (flags & CONF_FILES_FILTER_MASKED) {
-                masked = set_new(&path_hash_ops);
-                if (!masked)
-                        return -ENOMEM;
-        }
-
         STRV_FOREACH(p, dirs) {
-                r = files_add(fh, masked, suffix, root, flags, *p);
+                r = files_add(&fh, &masked, suffix, root, flags, *p);
                 if (r == -ENOMEM)
                         return r;
                 if (r < 0)
                         log_debug_errno(r, "Failed to search for files in %s, ignoring: %m", *p);
         }
 
-        files = hashmap_get_strv(fh);
+        sv = hashmap_get_strv(fh);
+        if (!sv)
+                return -ENOMEM;
+
+        /* The entries in the array given by hashmap_get_strv() are still owned by the hashmap. */
+        files = strv_copy(sv);
         if (!files)
                 return -ENOMEM;
 
-        typesafe_qsort(files, hashmap_size(fh), base_cmp);
-        *ret = files;
+        typesafe_qsort(files, strv_length(files), base_cmp);
+        *ret = TAKE_PTR(files);
 
         return 0;
 }
-- 
2.39.2