Hi, On Wed, 16 Sep 2020 12:04:55 +0200 Klemens Nanni <k...@openbsd.org> wrote: > Using the function verb would reads a bit clearer/more intuitive, > i.e.
Yes, "if (!rtable_exists($2))" seems better. >> @@ -5887,17 +5897,37 @@ rdomain_exists(u_int rdomain) >> >> len = sizeof(info); >> if (sysctl(mib, 6, &info, &len, NULL, 0) == -1) { >> - if (errno == ENOENT) { >> + if (errno == ENOENT) >> /* table nonexistent */ >> - return 0; >> - } >> - err(1, "%s", __func__); >> - } >> - if (info.rti_domainid == rdomain) { >> - found[rdomain] = 1; >> + domainid[rdomain] = RT_TABLEID_MAX; > This does not look correct, RT_TABLEID_MAX (255) is the biggest *valid* > id, so you cannot use it to denote a nonexistent routing table. Good catch. Thanks, > Perhaps use `static int domainid[RT_TABLEID_MAX+1]' and `-1' to reflect > ENOENT? New diff is using -1 for ENOENT. Also domainid == 0 is a valid domain id, but previous diff cannot make a cache of it since 0 is the default value. So new diff is doing - static u_int found[RT_TABLEID_MAX+1]; + static struct { + int found; + int domainid; + } rtables[RT_TABLEID_MAX+1]; to distinguish the default 0 and domainid 0. ok? Make pfctl check if the rtable really exists when parsing the config. Index: sbin/pfctl/parse.y =================================================================== RCS file: /cvs/src/sbin/pfctl/parse.y,v retrieving revision 1.701 diff -u -p -r1.701 parse.y --- sbin/pfctl/parse.y 28 Jan 2020 15:40:35 -0000 1.701 +++ sbin/pfctl/parse.y 16 Sep 2020 10:40:25 -0000 @@ -392,7 +392,9 @@ int invalid_redirect(struct node_host * u_int16_t parseicmpspec(char *, sa_family_t); int kw_casecmp(const void *, const void *); int map_tos(char *string, int *); +int get_domainid(u_int); int rdomain_exists(u_int); +int rtable_exists(u_int); int filteropts_to_rule(struct pf_rule *, struct filter_opts *); TAILQ_HEAD(loadanchorshead, loadanchors) @@ -1217,6 +1219,10 @@ antispoof_opt : LABEL label { yyerror("invalid rtable id"); YYERROR; } + else if (!rtable_exists($2)) { + yyerror("rtable %lld does not exist", $2); + YYERROR; + } antispoof_opts.rtableid = $2; } ; @@ -2001,6 +2007,10 @@ filter_opt : USER uids { yyerror("invalid rtable id"); YYERROR; } + else if (!rtable_exists($2)) { + yyerror("rtable %lld does not exist", $2); + YYERROR; + } filter_opts.rtableid = $2; } | DIVERTTO STRING PORT portplain { @@ -2475,7 +2485,7 @@ if_item : STRING { | RDOMAIN NUMBER { if ($2 < 0 || $2 > RT_TABLEID_MAX) yyerror("rdomain %lld outside range", $2); - else if (rdomain_exists($2) != 1) + else if (!rdomain_exists($2)) yyerror("rdomain %lld does not exist", $2); $$ = calloc(1, sizeof(struct node_if)); @@ -5868,36 +5878,60 @@ map_tos(char *s, int *val) } int -rdomain_exists(u_int rdomain) +get_domainid(u_int rtable) { size_t len; struct rt_tableinfo info; int mib[6]; - static u_int found[RT_TABLEID_MAX+1]; + static struct { + int found; + int domainid; + } rtables[RT_TABLEID_MAX+1]; - if (found[rdomain] == 1) - return 1; + if (rtables[rtable].found) + return rtables[rtable].domainid; mib[0] = CTL_NET; mib[1] = PF_ROUTE; mib[2] = 0; mib[3] = 0; mib[4] = NET_RT_TABLE; - mib[5] = rdomain; + mib[5] = rtable; len = sizeof(info); if (sysctl(mib, 6, &info, &len, NULL, 0) == -1) { - if (errno == ENOENT) { + if (errno == ENOENT) /* table nonexistent */ - return 0; - } - err(1, "%s", __func__); - } - if (info.rti_domainid == rdomain) { - found[rdomain] = 1; + rtables[rtable].domainid = -1; + else + err(1, "%s", __func__); + } else + rtables[rtable].domainid = info.rti_domainid; + rtables[rtable].found = 1; + + return rtables[rtable].domainid; +} + +int +rdomain_exists(u_int rdomain) +{ + int domainid; + + domainid = get_domainid(rdomain); + if (domainid == rdomain) return 1; - } /* rdomain is a table, but not an rdomain */ + return 0; +} + +int +rtable_exists(u_int rtable) +{ + int domainid; + + domainid = get_domainid(rtable); + if (domainid >= 0) + return 1; return 0; }