If the loop that registers the underlying tables detected an invalid
table spec (missing colon), it would free the partially constructed
pipemap or unionmap but not unregister any underlying tables that were
registered during previous iterations of the loop.  This would cause
their refcount increments to leak.  Fix the bug by checking every
underlying table spec before registering any of them.

I believe the effects of the bug are very minor; users are unlikely to
experience any notable negative consequences without this fix.  The
leak is one-time, and the returned surrogate dict is likely to render
their config useless anyway.  But I think it is still worth fixing:

  * The bug might raise alarms in static analysis tools or memory leak
    checkers like Valgrind.
  * The bug prevents the affected underlying tables from being
    properly closed before Postfix exits, which might matter for some
    table implementations.
  * This change makes it easier for potential contributors like me to
    understand the code.
---
 src/util/dict_pipe.c  | 17 ++++++++++++-----
 src/util/dict_union.c | 17 ++++++++++++-----
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/util/dict_pipe.c b/src/util/dict_pipe.c
index 8ce0faad7..f26474307 100644
--- a/src/util/dict_pipe.c
+++ b/src/util/dict_pipe.c
@@ -151,13 +151,10 @@ DICT   *dict_pipe_open(const char *name, int open_flags, 
int dict_flags)
                                        DICT_TYPE_PIPE));
 
     /*
-     * The least-trusted table in the pipeline determines the over-all trust
-     * level. The first table determines the pattern-matching flags.
+     * Check all underlying table specs before registering any of them to avoid
+     * leaking refcounts if one of them is bad.
      */
-    DICT_OWNER_AGGREGATE_INIT(aggr_owner);
     for (cpp = argv->argv; (dict_type_name = *cpp) != 0; cpp++) {
-       if (msg_verbose)
-           msg_info("%s: %s", myname, dict_type_name);
        if (strchr(dict_type_name, ':') == 0)
            DICT_PIPE_RETURN(dict_surrogate(DICT_TYPE_PIPE, name,
                                            open_flags, dict_flags,
@@ -165,6 +162,16 @@ DICT   *dict_pipe_open(const char *name, int open_flags, 
int dict_flags)
                                            "need \"%s:{type:name...}\"",
                                            DICT_TYPE_PIPE, name,
                                            DICT_TYPE_PIPE));
+    }
+
+    /*
+     * The least-trusted table in the pipeline determines the over-all trust
+     * level. The first table determines the pattern-matching flags.
+     */
+    DICT_OWNER_AGGREGATE_INIT(aggr_owner);
+    for (cpp = argv->argv; (dict_type_name = *cpp) != 0; cpp++) {
+       if (msg_verbose)
+           msg_info("%s: %s", myname, dict_type_name);
        if ((dict = dict_handle(dict_type_name)) == 0)
            dict = dict_open(dict_type_name, open_flags, dict_flags);
        dict_register(dict_type_name, dict);
diff --git a/src/util/dict_union.c b/src/util/dict_union.c
index 80df03b61..84f2b69b5 100644
--- a/src/util/dict_union.c
+++ b/src/util/dict_union.c
@@ -164,13 +164,10 @@ DICT   *dict_union_open(const char *name, int open_flags, 
int dict_flags)
                                         DICT_TYPE_UNION));
 
     /*
-     * The least-trusted table in the set determines the over-all trust
-     * level. The first table determines the pattern-matching flags.
+     * Check all underlying table specs before registering any of them to avoid
+     * leaking refcounts if one of them is bad.
      */
-    DICT_OWNER_AGGREGATE_INIT(aggr_owner);
     for (cpp = argv->argv; (dict_type_name = *cpp) != 0; cpp++) {
-       if (msg_verbose)
-           msg_info("%s: %s", myname, dict_type_name);
        if (strchr(dict_type_name, ':') == 0)
            DICT_UNION_RETURN(dict_surrogate(DICT_TYPE_UNION, name,
                                             open_flags, dict_flags,
@@ -178,6 +175,16 @@ DICT   *dict_union_open(const char *name, int open_flags, 
int dict_flags)
                                             "need \"%s:{type:name...}\"",
                                             DICT_TYPE_UNION, name,
                                             DICT_TYPE_UNION));
+    }
+
+    /*
+     * The least-trusted table in the set determines the over-all trust
+     * level. The first table determines the pattern-matching flags.
+     */
+    DICT_OWNER_AGGREGATE_INIT(aggr_owner);
+    for (cpp = argv->argv; (dict_type_name = *cpp) != 0; cpp++) {
+       if (msg_verbose)
+           msg_info("%s: %s", myname, dict_type_name);
        if ((dict = dict_handle(dict_type_name)) == 0)
            dict = dict_open(dict_type_name, open_flags, dict_flags);
        dict_register(dict_type_name, dict);
-- 
2.49.0

_______________________________________________
Postfix-devel mailing list -- postfix-devel@postfix.org
To unsubscribe send an email to postfix-devel-le...@postfix.org

Reply via email to