xshared: Introduce struct argv_store

The use of global variables in code around add_argv() is error-prone and
hard to follow. Replace them by a struct which functions will modify
instead of causing side-effects.

Given the lack of static variables, this effectively makes argv
construction code reentrant.

Signed-off-by: Phil Sutter <phil@nwl.cc>
Acked-by: Florian Westphal <fw@strlen.de>

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 50d0708eff1f3a3eb5011019638da8575a7c8d1c..b0a51d491c5081b2330c168a94be6c2f2af0424d 100644 (file)
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -94,6 +94,7 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
                        int argc, char *argv[])
 {
        struct xtc_handle *handle = NULL;
+       struct argv_store av_store = {};
        char buffer[10240];
        int c, lock;
        char curtable[XT_TABLE_MAXNAMELEN + 1] = {};
@@ -311,34 +312,31 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
                        ret = 1;
 
                } else if (in_table) {
-                       int a;
                        char *pcnt = NULL;
                        char *bcnt = NULL;
                        char *parsestart = buffer;
 
-                       add_argv(argv[0], 0);
-                       add_argv("-t", 0);
-                       add_argv(curtable, 0);
+                       add_argv(&av_store, argv[0], 0);
+                       add_argv(&av_store, "-t", 0);
+                       add_argv(&av_store, curtable, 0);
 
                        tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
                        if (counters && pcnt && bcnt) {
-                               add_argv("--set-counters", 0);
-                               add_argv(pcnt, 0);
-                               add_argv(bcnt, 0);
+                               add_argv(&av_store, "--set-counters", 0);
+                               add_argv(&av_store, pcnt, 0);
+                               add_argv(&av_store, bcnt, 0);
                        }
 
-                       add_param_to_argv(parsestart, line);
+                       add_param_to_argv(&av_store, parsestart, line);
 
                        DEBUGP("calling do_command(%u, argv, &%s, handle):\n",
-                               newargc, curtable);
+                               av_store.argc, curtable);
+                       debug_print_argv(&av_store);
 
-                       for (a = 0; a < newargc; a++)
-                               DEBUGP("argv[%u]: %s\n", a, newargv[a]);
+                       ret = cb->do_command(av_store.argc, av_store.argv,
+                                        &av_store.argv[2], &handle, true);
 
-                       ret = cb->do_command(newargc, newargv,
-                                        &newargv[2], &handle, true);
-
-                       free_argv();
+                       free_argv(&av_store);
                        fflush(stdout);
                }
                if (tablename && strcmp(tablename, curtable) != 0)

diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index eafee64f5e954ff85155d77b64b3a2a1d792d87c..98d03dda98d2b7ff38199293da6095cec44694c8 100644 (file)
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -440,7 +440,7 @@ do_rule_part(char *leveltag1, char *leveltag2, int part, int argc,
 }
 
 static int
-compareRules(void)
+compareRules(int newargc, char *newargv[], int oldargc, char *oldargv[])
 {
        /* Compare arguments up to -j or -g for match.
         * NOTE: We don't want to combine actions if there were no criteria
@@ -489,11 +489,13 @@ compareRules(void)
 
 /* has a nice parsed rule starting with -A */
 static void
-do_rule(char *pcnt, char *bcnt, int argc, char *argv[], int argvattr[])
+do_rule(char *pcnt, char *bcnt, int argc, char *argv[], int argvattr[],
+       int oldargc, char *oldargv[])
 {
        /* are these conditions the same as the previous rule?
         * If so, skip arg straight to -j or -g */
-       if (combine && argc > 2 && !isTarget(argv[2]) && compareRules()) {
+       if (combine && argc > 2 && !isTarget(argv[2]) &&
+           compareRules(argc, argv, oldargc, oldargv)) {
                xmlComment("Combine action from next rule");
        } else {
 
@@ -539,6 +541,7 @@ do_rule(char *pcnt, char *bcnt, int argc, char *argv[], int argvattr[])
 int
 iptables_xml_main(int argc, char *argv[])
 {
+       struct argv_store last_rule = {}, cur_rule = {};
        char buffer[10240];
        int c;
        FILE *in;
@@ -648,18 +651,16 @@ iptables_xml_main(int argc, char *argv[])
                        char *chain = NULL;
 
                        tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
-                       add_param_to_argv(parsestart, line);
+                       add_param_to_argv(&cur_rule, parsestart, line);
 
                        DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
-                              newargc, curTable);
+                              cur_rule.argc, curTable);
+                       debug_print_argv(&cur_rule);
 
-                       for (a = 0; a < newargc; a++)
-                               DEBUGP("argv[%u]: %s\n", a, newargv[a]);
-
-                       for (a = 1; a < newargc; a++) {
-                               if (strcmp(newargv[a - 1], "-A"))
+                       for (a = 1; a < cur_rule.argc; a++) {
+                               if (strcmp(cur_rule.argv[a - 1], "-A"))
                                        continue;
-                               chain = newargv[a];
+                               chain = cur_rule.argv[a];
                                break;
                        }
                        if (!chain) {
@@ -668,9 +669,10 @@ iptables_xml_main(int argc, char *argv[])
                                exit(1);
                        }
                        needChain(chain);// Should we explicitly look for -A
-                       do_rule(pcnt, bcnt, newargc, newargv, newargvattr);
+                       do_rule(pcnt, bcnt, cur_rule.argc, cur_rule.argv,
+                               cur_rule.argvattr, last_rule.argc, last_rule.argv);
 
-                       save_argv();
+                       save_argv(&last_rule, &cur_rule);
                        ret = 1;
                }
                if (!ret) {
@@ -687,7 +689,7 @@ iptables_xml_main(int argc, char *argv[])
 
        fclose(in);
        printf("</iptables-rules>\n");
-       free_argv();
+       free_argv(&last_rule);
 
        return 0;
 }

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 530ab4c1286617f8c090be9851b70eea312a6add..97f1b5d22fdbede092a9bf1fdcee7c7259c0ca05 100644 (file)
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -417,56 +417,48 @@ inline bool xs_has_arg(int argc, char *argv[])
               argv[optind][0] != '!';
 }
 
-/* global new argv and argc */
-char *newargv[255];
-int newargc = 0;
-
-/* saved newargv and newargc from save_argv() */
-char *oldargv[255];
-int oldargc = 0;
-
-/* arg meta data, were they quoted, frinstance */
-int newargvattr[255];
-
-/* function adding one argument to newargv, updating newargc
- * returns true if argument added, false otherwise */
-int add_argv(const char *what, int quoted)
+/* function adding one argument to store, updating argc
+ * returns if argument added, does not return otherwise */
+void add_argv(struct argv_store *store, const char *what, int quoted)
 {
        DEBUGP("add_argv: %s\n", what);
-       if (what && newargc + 1 < ARRAY_SIZE(newargv)) {
-               newargv[newargc] = strdup(what);
-               newargvattr[newargc] = quoted;
-               newargv[++newargc] = NULL;
-               return 1;
-       } else {
+
+       if (store->argc + 1 >= MAX_ARGC)
                xtables_error(PARAMETER_PROBLEM,
                              "Parser cannot handle more arguments\n");
-       }
+       if (!what)
+               xtables_error(PARAMETER_PROBLEM,
+                             "Trying to store NULL argument\n");
+
+       store->argv[store->argc] = strdup(what);
+       store->argvattr[store->argc] = quoted;
+       store->argv[++store->argc] = NULL;
 }
 
-void free_argv(void)
+void free_argv(struct argv_store *store)
 {
-       while (newargc)
-               free(newargv[--newargc]);
-       while (oldargc)
-               free(oldargv[--oldargc]);
+       while (store->argc) {
+               store->argc--;
+               free(store->argv[store->argc]);
+               store->argvattr[store->argc] = 0;
+       }
 }
 
 /* Save parsed rule for comparison with next rule to perform action aggregation
  * on duplicate conditions.
  */
-void save_argv(void)
+void save_argv(struct argv_store *dst, struct argv_store *src)
 {
-       unsigned int i;
+       int i;
 
-       while (oldargc)
-               free(oldargv[--oldargc]);
-
-       oldargc = newargc;
-       newargc = 0;
-       for (i = 0; i < oldargc; i++) {
-               oldargv[i] = newargv[i];
+       free_argv(dst);
+       for (i = 0; i < src->argc; i++) {
+               dst->argvattr[i] = src->argvattr[i];
+               dst->argv[i] = src->argv[i];
+               src->argv[i] = NULL;
        }
+       dst->argc = src->argc;
+       src->argc = 0;
 }
 
 struct xt_param_buf {
@@ -482,7 +474,7 @@ static void add_param(struct xt_param_buf *param, const char *curchar)
                              "Parameter too long!");
 }
 
-void add_param_to_argv(char *parsestart, int line)
+void add_param_to_argv(struct argv_store *store, char *parsestart, int line)
 {
        int quote_open = 0, escaped = 0, quoted = 0;
        struct xt_param_buf param = {};
@@ -534,12 +526,22 @@ void add_param_to_argv(char *parsestart, int line)
                }
 
                param.buffer[param.len] = '\0';
-               add_argv(param.buffer, quoted);
+               add_argv(store, param.buffer, quoted);
                param.len = 0;
                quoted = 0;
        }
 }
 
+#ifdef DEBUG
+void debug_print_argv(struct argv_store *store)
+{
+       int i;
+
+       for (i = 0; i < store->argc; i++)
+               fprintf(stderr, "argv[%d]: %s\n", i, store->argv[i]);
+}
+#endif
+
 static const char *ipv4_addr_to_string(const struct in_addr *addr,
                                       const struct in_addr *mask,
                                       unsigned int format)

diff --git a/iptables/xshared.h b/iptables/xshared.h
index 21f4e8fdee67ca851efb2c08c11b7314ef6ec41d..64b7e8fc4b6902d140f31929014884a816d6e9ea 100644 (file)
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -156,18 +156,22 @@ bool xs_has_arg(int argc, char *argv[]);
 
 extern const struct xtables_afinfo *afinfo;
 
-extern char *newargv[];
-extern int newargc;
-
-extern char *oldargv[];
-extern int oldargc;
-
-extern int newargvattr[];
+#define MAX_ARGC       255
+struct argv_store {
+       int argc;
+       char *argv[MAX_ARGC];
+       int argvattr[MAX_ARGC];
+};
 
-int add_argv(const char *what, int quoted);
-void free_argv(void);
-void save_argv(void);
-void add_param_to_argv(char *parsestart, int line);
+void add_argv(struct argv_store *store, const char *what, int quoted);
+void free_argv(struct argv_store *store);
+void save_argv(struct argv_store *dst, struct argv_store *src);
+void add_param_to_argv(struct argv_store *store, char *parsestart, int line);
+#ifdef DEBUG
+void debug_print_argv(struct argv_store *store);
+#else
+#  define debug_print_argv(...) /* nothing */
+#endif
 
 void print_ipv4_addresses(const struct ipt_entry *fw, unsigned int format);
 void print_ipv6_addresses(const struct ip6t_entry *fw6, unsigned int format);

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 1c7d5da52df64fe36bc79c30574f8287bfbc91e6..8d6cb7a97ea37d1343d070ffe7986cb89ca4b808 100644 (file)
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -73,6 +73,7 @@ void xtables_restore_parse(struct nft_handle *h,
                           const struct nft_xt_restore_cb *cb)
 {
        const struct builtin_table *curtable = NULL;
+       struct argv_store av_store = {};
        char buffer[10240];
        int in_table = 0;
 
@@ -209,35 +210,29 @@ void xtables_restore_parse(struct nft_handle *h,
                        }
                        ret = 1;
                } else if (in_table) {
-                       int a;
                        char *pcnt = NULL;
                        char *bcnt = NULL;
                        char *parsestart = buffer;
 
-                       /* reset the newargv */
-                       newargc = 0;
-
-                       add_argv(xt_params->program_name, 0);
-                       add_argv("-t", 0);
-                       add_argv(curtable->name, 0);
+                       add_argv(&av_store, xt_params->program_name, 0);
+                       add_argv(&av_store, "-t", 0);
+                       add_argv(&av_store, curtable->name, 0);
 
                        tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
                        if (counters && pcnt && bcnt) {
-                               add_argv("--set-counters", 0);
-                               add_argv(pcnt, 0);
-                               add_argv(bcnt, 0);
+                               add_argv(&av_store, "--set-counters", 0);
+                               add_argv(&av_store, pcnt, 0);
+                               add_argv(&av_store, bcnt, 0);
                        }
 
-                       add_param_to_argv(parsestart, line);
+                       add_param_to_argv(&av_store, parsestart, line);
 
                        DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
-                               newargc, curtable->name);
-
-                       for (a = 0; a < newargc; a++)
-                               DEBUGP("argv[%u]: %s\n", a, newargv[a]);
+                              av_store.argc, curtable->name);
+                       debug_print_argv(&av_store);
 
-                       ret = cb->do_command(h, newargc, newargv,
-                                           &newargv[2], true);
+                       ret = cb->do_command(h, av_store.argc, av_store.argv,
+                                           &av_store.argv[2], true);
                        if (ret < 0) {
                                if (cb->abort)
                                        ret = cb->abort(h);
@@ -251,7 +246,7 @@ void xtables_restore_parse(struct nft_handle *h,
                                exit(1);
                        }
 
-                       free_argv();
+                       free_argv(&av_store);
                        fflush(stdout);
                }
                if (p->tablename && curtable &&