Richard Hansen via Postfix-devel:
> 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:
Nice find, but the impact is easily overstated. Postfix lookup
tables are opened once during process initialization, so these are
one-time errors.
These tables are configured statically by a (possibly confused)
system adminstrator. Dynamically opening tables with (type, name)
based on user input would open a huge can of worms.
Thus, the only adversary here is the person between the chair and
the keyboard. For this reason a one-time leak involving an error
code path is a low priority.
> * The bug might raise alarms in static analysis tools or memory leak
> checkers like Valgrind.
How would that wotk? The table has a non-zero refceount, and will
still be referenced (!) through a static variable (the pointer to
the hash table that maps table names to dictionary instances).
> * The bug prevents the affected underlying tables from being
> properly closed before Postfix exits, which might matter for some
> table implementations.
A non-problem, because Postfix programs already 'exit' (normally
or abnormally) without closing all tables. It also helps that
union:{} and pipe:{} opens tables read-only.
> * This change makes it easier for potential contributors like me to
> understand the code.
Fortunately, the change is not invasive, so that it can be adopted
without any automated tests. But we're approaching the moment that
I will insist on a test to verify that the promised behavior is
actually implemented (and that it remains implemented as Postfix
evolves).
Such a test would look like:
- Normal case: instantiate a composite table, close the table,
and then verify that dict_handle() returns NULL for the
component tables.
- Error case(s): instantiate a composite table with a name syntax
error. Verify that the dict_handle() returns NULL for all expected
component tables.
It may be worthwhile to change the dict_union/pipe implementations
to identify tables not only by name and type, but also by dict_flags
in the way that maps_create() does. I'll implement that separately.
This patch is accepted without tests.
Wietse
_______________________________________________
Postfix-devel mailing list -- postfix-devel@postfix.org
To unsubscribe send an email to postfix-devel-le...@postfix.org
