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;
}
