xshared: Fix for memleak in option merging with ebtables

The crucial difference in ebtables is that all extensions are loaded up
front instead of while parsing -m/-j flags. Since this loading of all
extensions before every call to do_parse() is pointless overhead (cf.
ebtables-restore), other tools' mechanism of freeing all merged options
in xtables_free_opts() after handling each command and resetting
xt_params->opts at the start of the parser loop is problematic.

Fixed commit entailed a hack to defeat the xt_params->opts happening at
start of do_parse() by assigning to xt_params->orig_opts after loading
all extensions. This approach caused a memleak though since
xtables_free_opts() called from xtables_merge_options() will free the
opts pointer only if it differs from orig_opts.

Resolve this via a different approach which eliminates the
xt_params->opts reset at the start of do_parse():

Make xt_params->opts be NULL until the first extension is loaded. Option
merging in command_match() and command_jump() tolerates a NULL pointer
there after minimal adjustment. Deinit in xtables_free_opts() is already
fine as it (re)turns xt_params->opts to a NULL pointer. With do_parse()
expecting that and falling back to xt_params->orig_opts, no explicit
initialization is required anymore and thus ebtables' init is not
mangled by accident.

A critical part is that do_parse() checks xt_params->opts pointer upon
each call to getopt_long() as it may get assigned while parsing.

Fixes: 58d364c7120b5 ("ebtables: Use do_parse() from xshared")
Signed-off-by: Phil Sutter <phil@nwl.cc>

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 43fa929df76763af0c7d1818f96cae890d6f7609..7d073891ed5c324158eb841dffcf4e6faac6b66f 100644 (file)
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -798,6 +798,9 @@ static void command_match(struct iptables_command_state *cs, bool invert)
        else if (m->extra_opts != NULL)
                opts = xtables_merge_options(xt_params->orig_opts, opts,
                                             m->extra_opts, &m->option_offset);
+       else
+               return;
+
        if (opts == NULL)
                xtables_error(OTHER_PROBLEM, "can't alloc memory!");
        xt_params->opts = opts;
@@ -856,10 +859,13 @@ void command_jump(struct iptables_command_state *cs, const char *jumpto)
                opts = xtables_options_xfrm(xt_params->orig_opts, opts,
                                            cs->target->x6_options,
                                            &cs->target->option_offset);
-       else
+       else if (cs->target->extra_opts != NULL)
                opts = xtables_merge_options(xt_params->orig_opts, opts,
                                             cs->target->extra_opts,
                                             &cs->target->option_offset);
+       else
+               return;
+
        if (opts == NULL)
                xtables_error(OTHER_PROBLEM, "can't alloc memory!");
        xt_params->opts = opts;
@@ -1484,10 +1490,10 @@ void do_parse(int argc, char *argv[],
           demand-load a protocol. */
        opterr = 0;
 
-       xt_params->opts = xt_params->orig_opts;
        while ((cs->c = getopt_long(argc, argv,
                                    optstring_lookup(afinfo->family),
-                                   xt_params->opts, NULL)) != -1) {
+                                   xt_params->opts ?: xt_params->orig_opts,
+                                   NULL)) != -1) {
                switch (cs->c) {
                        /*
                         * Command selection

diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index d197b37e81e9e5ba49f466dd41b51d0ca2c0a964..51c699defb047614ce8b0cd23d6d866b6bc1e3ba 100644 (file)
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -298,11 +298,14 @@ static void ebt_load_match(const char *name)
        xs_init_match(m);
 
        if (m->x6_options != NULL)
-               opts = xtables_options_xfrm(opts, NULL,
+               opts = xtables_options_xfrm(xt_params->orig_opts, opts,
                                            m->x6_options, &m->option_offset);
        else if (m->extra_opts != NULL)
-               opts = xtables_merge_options(opts, NULL,
+               opts = xtables_merge_options(xt_params->orig_opts, opts,
                                             m->extra_opts, &m->option_offset);
+       else
+               return;
+
        if (opts == NULL)
                xtables_error(OTHER_PROBLEM, "Can't alloc memory");
        xt_params->opts = opts;
@@ -332,11 +335,16 @@ static void ebt_load_watcher(const char *name)
        xs_init_target(watcher);
 
        if (watcher->x6_options != NULL)
-               opts = xtables_options_xfrm(opts, NULL, watcher->x6_options,
+               opts = xtables_options_xfrm(xt_params->orig_opts, opts,
+                                           watcher->x6_options,
                                            &watcher->option_offset);
        else if (watcher->extra_opts != NULL)
-               opts = xtables_merge_options(opts, NULL, watcher->extra_opts,
+               opts = xtables_merge_options(xt_params->orig_opts, opts,
+                                            watcher->extra_opts,
                                             &watcher->option_offset);
+       else
+               return;
+
        if (opts == NULL)
                xtables_error(OTHER_PROBLEM, "Can't alloc memory");
        xt_params->opts = opts;
@@ -344,7 +352,6 @@ static void ebt_load_watcher(const char *name)
 
 static void ebt_load_match_extensions(void)
 {
-       xt_params->opts = ebt_original_options;
        ebt_load_match("802_3");
        ebt_load_match("arp");
        ebt_load_match("ip");
@@ -358,10 +365,6 @@ static void ebt_load_match_extensions(void)
 
        ebt_load_watcher("log");
        ebt_load_watcher("nflog");
-
-       /* assign them back so do_parse() may
-        * reset opts to orig_opts upon each call */
-       xt_params->orig_opts = xt_params->opts;
 }
 
 void ebt_add_match(struct xtables_match *m,
@@ -531,7 +534,6 @@ int nft_init_eb(struct nft_handle *h, const char *pname)
 
 void nft_fini_eb(struct nft_handle *h)
 {
-       struct option *opts = xt_params->opts;
        struct xtables_match *match;
        struct xtables_target *target;
 
@@ -542,8 +544,7 @@ void nft_fini_eb(struct nft_handle *h)
                free(target->t);
        }
 
-       if (opts != ebt_original_options)
-               free(opts);
+       free(xt_params->opts);
 
        nft_fini(h);
        xtables_fini();

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 856bfae804ea9bae4ffb5575ed53289d8aa1cbe5..ae3ff25a3a8769fbe836d54edd5c929982192fe2 100644 (file)
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -111,10 +111,8 @@ void basic_exit_err(enum xtables_exittype status, const char *msg, ...)
 
 void xtables_free_opts(int unused)
 {
-       if (xt_params->opts != xt_params->orig_opts) {
-               free(xt_params->opts);
-               xt_params->opts = NULL;
-       }
+       free(xt_params->opts);
+       xt_params->opts = NULL;
 }
 
 struct option *xtables_merge_options(struct option *orig_opts,