| Message ID | 20191212171455.83382-1-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] main: allow for getopt parser from top-level scope only | expand |
Hi Pablo, On Thu, Dec 12, 2019 at 06:14:55PM +0100, Pablo Neira Ayuso wrote: [...] > diff --git a/src/main.c b/src/main.c > index fde8b15c5870..c96953e3cd2f 100644 > --- a/src/main.c > +++ b/src/main.c > @@ -202,29 +202,107 @@ static const struct { > }, > }; > > +struct nft_opts { > + char **argv; > + int argc; > +}; > + > +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) > +{ > + uint32_t scope = 0; > + char *new_argv; > + int i; > + > + opts->argv = calloc(argc + 1, sizeof(char *)); > + if (!opts->argv) > + return -1; > + > + for (i = 0; i < argc; i++) { > + if (scope > 0) { > + if (argv[i][0] == '-') { > + new_argv = malloc(strlen(argv[i]) + 2); > + if (!new_argv) > + return -1; > + > + sprintf(new_argv, "\\-%s", &argv[i][1]); > + opts->argv[opts->argc++] = new_argv; > + continue; > + } > + } else if (argv[i][0] == '{') { > + scope++; > + } else if (argv[i][0] == '}') { > + scope--; > + } This first char check is not reliable, bison accepts commands which lack spaces in the relevant places: | # nft add chain inet t c{ type filter hook input priority filter\; } | # echo $? | 0 Cheers, Phil
On Thursday 2019-12-12 18:45, Phil Sutter wrote: >[...] >> diff --git a/src/main.c b/src/main.c >> index fde8b15c5870..c96953e3cd2f 100644 >> --- a/src/main.c >> +++ b/src/main.c >> +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) >> +{ >> + uint32_t scope = 0; >> + char *new_argv; >> + int i; >> + >> + opts->argv = calloc(argc + 1, sizeof(char *)); >> + if (!opts->argv) >> + return -1; >> + >> + for (i = 0; i < argc; i++) { >> + if (scope > 0) { >> + if (argv[i][0] == '-') { >> + new_argv = malloc(strlen(argv[i]) + 2); [...] Or simply stop taking options after the first-non option. This is declared POSIX behavior, and, for glibc, it only needs the POSIXLY_CORRECT environment variable, which can be set ahead of getopt()/getopt_long() call and unset afterwards again.
On Thu, Dec 12, 2019 at 06:45:35PM +0100, Phil Sutter wrote: > Hi Pablo, > > On Thu, Dec 12, 2019 at 06:14:55PM +0100, Pablo Neira Ayuso wrote: > [...] > > diff --git a/src/main.c b/src/main.c > > index fde8b15c5870..c96953e3cd2f 100644 > > --- a/src/main.c > > +++ b/src/main.c > > @@ -202,29 +202,107 @@ static const struct { > > }, > > }; > > > > +struct nft_opts { > > + char **argv; > > + int argc; > > +}; > > + > > +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) > > +{ > > + uint32_t scope = 0; > > + char *new_argv; > > + int i; > > + > > + opts->argv = calloc(argc + 1, sizeof(char *)); > > + if (!opts->argv) > > + return -1; > > + > > + for (i = 0; i < argc; i++) { > > + if (scope > 0) { > > + if (argv[i][0] == '-') { > > + new_argv = malloc(strlen(argv[i]) + 2); > > + if (!new_argv) > > + return -1; > > + > > + sprintf(new_argv, "\\-%s", &argv[i][1]); > > + opts->argv[opts->argc++] = new_argv; > > + continue; > > + } > > + } else if (argv[i][0] == '{') { > > + scope++; > > + } else if (argv[i][0] == '}') { > > + scope--; > > + } > > This first char check is not reliable, bison accepts commands which lack > spaces in the relevant places: > > | # nft add chain inet t c{ type filter hook input priority filter\; } > | # echo $? > | 0 Yes, it won't catch that case. Do you think it is worth going further in this preprocessing? Incremental patch on top of this one is attached to this email.
On Thu, Dec 12, 2019 at 07:27:45PM +0100, Jan Engelhardt wrote: > On Thursday 2019-12-12 18:45, Phil Sutter wrote: > >[...] > >> diff --git a/src/main.c b/src/main.c > >> index fde8b15c5870..c96953e3cd2f 100644 > >> --- a/src/main.c > >> +++ b/src/main.c > >> +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) > >> +{ > >> + uint32_t scope = 0; > >> + char *new_argv; > >> + int i; > >> + > >> + opts->argv = calloc(argc + 1, sizeof(char *)); > >> + if (!opts->argv) > >> + return -1; > >> + > >> + for (i = 0; i < argc; i++) { > >> + if (scope > 0) { > >> + if (argv[i][0] == '-') { > >> + new_argv = malloc(strlen(argv[i]) + 2); > [...] > > Or simply stop taking options after the first-non option. > This is declared POSIX behavior, and, for glibc, it only needs the > POSIXLY_CORRECT environment variable, which can be set ahead of > getopt()/getopt_long() call and unset afterwards again. I think we tried that already, IIRC it breaks: nft list ruleset -a which is in the test scripts. The most sane approach from programmer perspective is to force users to place options upfront. Otherwise, this needs this ugly preprocessing which gives a bit more flexibility to users in turn.
On Thursday 2019-12-12 19:33, Pablo Neira Ayuso wrote: >> >> Or simply stop taking options after the first-non option. >> This is declared POSIX behavior, and, for glibc, it only needs the >> POSIXLY_CORRECT environment variable, which can be set ahead of >> getopt()/getopt_long() call and unset afterwards again. > >I think we tried that already, IIRC it breaks: nft list ruleset -a >which is in the test scripts. Yes, it will need to be rewritten as `nft -a list ruleset`. But.. you seem to be just fine with that, based on: >The most sane approach from programmer perspective is to force users >to place options upfront.
Hi, On Thu, Dec 12, 2019 at 07:28:11PM +0100, Pablo Neira Ayuso wrote: > On Thu, Dec 12, 2019 at 06:45:35PM +0100, Phil Sutter wrote: > > On Thu, Dec 12, 2019 at 06:14:55PM +0100, Pablo Neira Ayuso wrote: > > [...] > > > diff --git a/src/main.c b/src/main.c > > > index fde8b15c5870..c96953e3cd2f 100644 > > > --- a/src/main.c > > > +++ b/src/main.c > > > @@ -202,29 +202,107 @@ static const struct { > > > }, > > > }; > > > > > > +struct nft_opts { > > > + char **argv; > > > + int argc; > > > +}; > > > + > > > +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) > > > +{ > > > + uint32_t scope = 0; > > > + char *new_argv; > > > + int i; > > > + > > > + opts->argv = calloc(argc + 1, sizeof(char *)); > > > + if (!opts->argv) > > > + return -1; > > > + > > > + for (i = 0; i < argc; i++) { > > > + if (scope > 0) { > > > + if (argv[i][0] == '-') { > > > + new_argv = malloc(strlen(argv[i]) + 2); > > > + if (!new_argv) > > > + return -1; > > > + > > > + sprintf(new_argv, "\\-%s", &argv[i][1]); > > > + opts->argv[opts->argc++] = new_argv; > > > + continue; > > > + } > > > + } else if (argv[i][0] == '{') { > > > + scope++; > > > + } else if (argv[i][0] == '}') { > > > + scope--; > > > + } > > > > This first char check is not reliable, bison accepts commands which lack > > spaces in the relevant places: > > > > | # nft add chain inet t c{ type filter hook input priority filter\; } > > | # echo $? > > | 0 > > Yes, it won't catch that case. Do you think it is worth going further > in this preprocessing? What about a different approach, namely to iterate over argv in reverse, reordering those *argv until **argv != '-'? One would have to make sure not to mess ordering, but that should be the only requirement to get expected results in any situation. Cheers, Phil
On Fri, Dec 13, 2019 at 11:33:45AM +0100, Phil Sutter wrote: > Hi, > > On Thu, Dec 12, 2019 at 07:28:11PM +0100, Pablo Neira Ayuso wrote: > > On Thu, Dec 12, 2019 at 06:45:35PM +0100, Phil Sutter wrote: > > > On Thu, Dec 12, 2019 at 06:14:55PM +0100, Pablo Neira Ayuso wrote: > > > [...] > > > > diff --git a/src/main.c b/src/main.c > > > > index fde8b15c5870..c96953e3cd2f 100644 > > > > --- a/src/main.c > > > > +++ b/src/main.c > > > > @@ -202,29 +202,107 @@ static const struct { > > > > }, > > > > }; > > > > > > > > +struct nft_opts { > > > > + char **argv; > > > > + int argc; > > > > +}; > > > > + > > > > +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) > > > > +{ > > > > + uint32_t scope = 0; > > > > + char *new_argv; > > > > + int i; > > > > + > > > > + opts->argv = calloc(argc + 1, sizeof(char *)); > > > > + if (!opts->argv) > > > > + return -1; > > > > + > > > > + for (i = 0; i < argc; i++) { > > > > + if (scope > 0) { > > > > + if (argv[i][0] == '-') { > > > > + new_argv = malloc(strlen(argv[i]) + 2); > > > > + if (!new_argv) > > > > + return -1; > > > > + > > > > + sprintf(new_argv, "\\-%s", &argv[i][1]); > > > > + opts->argv[opts->argc++] = new_argv; > > > > + continue; > > > > + } > > > > + } else if (argv[i][0] == '{') { > > > > + scope++; > > > > + } else if (argv[i][0] == '}') { > > > > + scope--; > > > > + } > > > > > > This first char check is not reliable, bison accepts commands which lack > > > spaces in the relevant places: > > > > > > | # nft add chain inet t c{ type filter hook input priority filter\; } > > > | # echo $? > > > | 0 > > > > Yes, it won't catch that case. Do you think it is worth going further > > in this preprocessing? > > What about a different approach, namely to iterate over argv in reverse, > reordering those *argv until **argv != '-'? One would have to make sure > not to mess ordering, but that should be the only requirement to get > expected results in any situation. That's another possibility, yes: argv[i][strlen(argv[i]) - 1] == '{'
On Fri, Dec 13, 2019 at 11:36:24AM +0100, Pablo Neira Ayuso wrote: > On Fri, Dec 13, 2019 at 11:33:45AM +0100, Phil Sutter wrote: > > Hi, > > > > On Thu, Dec 12, 2019 at 07:28:11PM +0100, Pablo Neira Ayuso wrote: > > > On Thu, Dec 12, 2019 at 06:45:35PM +0100, Phil Sutter wrote: > > > > On Thu, Dec 12, 2019 at 06:14:55PM +0100, Pablo Neira Ayuso wrote: > > > > [...] > > > > > diff --git a/src/main.c b/src/main.c > > > > > index fde8b15c5870..c96953e3cd2f 100644 > > > > > --- a/src/main.c > > > > > +++ b/src/main.c > > > > > @@ -202,29 +202,107 @@ static const struct { > > > > > }, > > > > > }; > > > > > > > > > > +struct nft_opts { > > > > > + char **argv; > > > > > + int argc; > > > > > +}; > > > > > + > > > > > +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) > > > > > +{ > > > > > + uint32_t scope = 0; > > > > > + char *new_argv; > > > > > + int i; > > > > > + > > > > > + opts->argv = calloc(argc + 1, sizeof(char *)); > > > > > + if (!opts->argv) > > > > > + return -1; > > > > > + > > > > > + for (i = 0; i < argc; i++) { > > > > > + if (scope > 0) { > > > > > + if (argv[i][0] == '-') { > > > > > + new_argv = malloc(strlen(argv[i]) + 2); > > > > > + if (!new_argv) > > > > > + return -1; > > > > > + > > > > > + sprintf(new_argv, "\\-%s", &argv[i][1]); > > > > > + opts->argv[opts->argc++] = new_argv; > > > > > + continue; > > > > > + } > > > > > + } else if (argv[i][0] == '{') { > > > > > + scope++; > > > > > + } else if (argv[i][0] == '}') { > > > > > + scope--; > > > > > + } > > > > > > > > This first char check is not reliable, bison accepts commands which lack > > > > spaces in the relevant places: > > > > > > > > | # nft add chain inet t c{ type filter hook input priority filter\; } > > > > | # echo $? > > > > | 0 > > > > > > Yes, it won't catch that case. Do you think it is worth going further > > > in this preprocessing? > > > > What about a different approach, namely to iterate over argv in reverse, > > reordering those *argv until **argv != '-'? One would have to make sure > > not to mess ordering, but that should be the only requirement to get > > expected results in any situation. > > That's another possibility, yes: > > argv[i][strlen(argv[i]) - 1] == '{' Bison doesn't allow for easy cheats: | # nft add chain inet t c{type filter hook input priority filter\; } | # echo $? | 0 Let me quickly hack my reordering idea to see how much of a mess it will become. Cheers, Phil
diff --git a/src/main.c b/src/main.c index fde8b15c5870..c96953e3cd2f 100644 --- a/src/main.c +++ b/src/main.c @@ -202,29 +202,107 @@ static const struct { }, }; +struct nft_opts { + char **argv; + int argc; +}; + +static int nft_opts_init(int argc, char * const argv[], struct nft_opts *opts) +{ + uint32_t scope = 0; + char *new_argv; + int i; + + opts->argv = calloc(argc + 1, sizeof(char *)); + if (!opts->argv) + return -1; + + for (i = 0; i < argc; i++) { + if (scope > 0) { + if (argv[i][0] == '-') { + new_argv = malloc(strlen(argv[i]) + 2); + if (!new_argv) + return -1; + + sprintf(new_argv, "\\-%s", &argv[i][1]); + opts->argv[opts->argc++] = new_argv; + continue; + } + } else if (argv[i][0] == '{') { + scope++; + } else if (argv[i][0] == '}') { + scope--; + } + + opts->argv[opts->argc++] = strdup(argv[i]); + } + + return 0; +} + +static int nft_opts_update(struct nft_opts *opts) +{ + char *new_argv; + int i; + + for (i = 0; i < opts->argc; i++) { + if (opts->argv[i][0] == '\\') { + new_argv = malloc(strlen(opts->argv[i]) + 1); + if (!new_argv) + return -1; + + sprintf(new_argv, "-%s", &opts->argv[i][2]); + free(opts->argv[i]); + opts->argv[i] = new_argv; + } + } + + return 0; +} + +static void nft_opts_release(struct nft_opts *opts) +{ + int i; + + for (i = 0; i < opts->argc; i++) + free(opts->argv[i]); + + free(opts->argv); +} + int main(int argc, char * const *argv) { char *buf = NULL, *filename = NULL; unsigned int output_flags = 0; + struct nft_opts opts = {}; bool interactive = false; unsigned int debug_mask; unsigned int len; int i, val, rc; + if (nft_opts_init(argc, argv, &opts) < 0) { + nft_opts_release(&opts); + fprintf(stderr, "OOM\n"); + exit(EXIT_FAILURE); + } + nft = nft_ctx_new(NFT_CTX_DEFAULT); while (1) { - val = getopt_long(argc, argv, OPTSTRING, options, NULL); + val = getopt_long(opts.argc, opts.argv, OPTSTRING, options, + NULL); if (val == -1) break; switch (val) { case OPT_HELP: show_help(argv[0]); + nft_opts_release(&opts); exit(EXIT_SUCCESS); case OPT_VERSION: printf("%s v%s (%s)\n", PACKAGE_NAME, PACKAGE_VERSION, RELEASE_NAME); + nft_opts_release(&opts); exit(EXIT_SUCCESS); case OPT_CHECK: nft_ctx_set_dry_run(nft, true); @@ -240,6 +318,7 @@ int main(int argc, char * const *argv) fprintf(stderr, "Failed to add include path '%s'\n", optarg); + nft_opts_release(&opts); exit(EXIT_FAILURE); } break; @@ -275,6 +354,7 @@ int main(int argc, char * const *argv) if (i == array_size(debug_param)) { fprintf(stderr, "invalid debug parameter `%s'\n", optarg); + nft_opts_release(&opts); exit(EXIT_FAILURE); } @@ -295,6 +375,7 @@ int main(int argc, char * const *argv) output_flags |= NFT_CTX_OUTPUT_JSON; #else fprintf(stderr, "JSON support not compiled-in\n"); + nft_opts_release(&opts); exit(EXIT_FAILURE); #endif break; @@ -314,24 +395,32 @@ int main(int argc, char * const *argv) output_flags |= NFT_CTX_OUTPUT_TERSE; break; case OPT_INVALID: + nft_opts_release(&opts); exit(EXIT_FAILURE); } } nft_ctx_output_set_flags(nft, output_flags); + if (nft_opts_update(&opts) < 0) { + nft_opts_release(&opts); + fprintf(stderr, "OOM\n"); + exit(EXIT_FAILURE); + } + if (optind != argc) { for (len = 0, i = optind; i < argc; i++) - len += strlen(argv[i]) + strlen(" "); + len += strlen(opts.argv[i]) + strlen(" "); buf = calloc(1, len); if (buf == NULL) { fprintf(stderr, "%s:%u: Memory allocation failure\n", __FILE__, __LINE__); + nft_opts_release(&opts); exit(EXIT_FAILURE); } for (i = optind; i < argc; i++) { - strcat(buf, argv[i]); + strcat(buf, opts.argv[i]); if (i + 1 < argc) strcat(buf, " "); } @@ -342,14 +431,17 @@ int main(int argc, char * const *argv) if (cli_init(nft) < 0) { fprintf(stderr, "%s: interactive CLI not supported in this build\n", argv[0]); + nft_opts_release(&opts); exit(EXIT_FAILURE); } return EXIT_SUCCESS; } else { fprintf(stderr, "%s: no command specified\n", argv[0]); + nft_opts_release(&opts); exit(EXIT_FAILURE); } + nft_opts_release(&opts); free(buf); nft_ctx_free(nft);
This patch restricts the getopt parser to the top-level scope. This is implicitly providing a fix for the following error reporting: # nft add chain x k { type filter hook input priority -10\; } nft: invalid option -- '1' When defining basechains using a negative priority number. This patch is processing the input to escape all dash (-) after leaving the top-level scope (ie. {), then it removes the escaping before entering the bison parser. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- src/main.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 95 insertions(+), 3 deletions(-)