When creating tables inside anchors, pfctl warned about namespace

collisions with global tables, but only in certain cases and with
limited information sometimes leaving users clueless.

Deferring the check to process_tabledefs() where tables are eventually
created, both anchor and table name are known which allows for checking
all existing anchors.

With this, warn on all duplicates even in dry-runs (`-n') and print
quoted names so they can be copied to fix configurations right away.

No functional change in parsing or ruleset production.

Discussed with and OK sashan

4 files changed, 14 insertions, 25 deletions

diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index 3447119d708..4b94b68de4f 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parse.y,v 1.688 2018/11/15 03:22:01 dlg Exp $	*/
+/*	$OpenBSD: parse.y,v 1.689 2019/01/11 01:56:54 kn Exp $	*/
 
 /*
  * Copyright (c) 2001 Markus Friedl.  All rights reserved.
@@ -4075,6 +4075,7 @@ process_tabledef(char *name, struct table_opts *opts, int popts)
 	if (pf->opts & PF_OPT_VERBOSE)
 		print_tabledef(name, opts->flags, opts->init_addr,
 		    &opts->init_nodes);
+	warn_duplicate_tables(name, pf->anchor->path);
 	if (!(pf->opts & PF_OPT_NOACTION) &&
 	    pfctl_define_table(name, opts->flags, opts->init_addr,
 	    pf->anchor->path, &ab, pf->anchor->ruleset.tticket)) {
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 349d73ac2bd..362fc58e7b8 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl.c,v 1.363 2019/01/10 22:22:51 kn Exp $ */
+/*	$OpenBSD: pfctl.c,v 1.364 2019/01/11 01:56:54 kn Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -2693,8 +2693,6 @@ main(int argc, char *argv[])
 		if (pfctl_rules(dev, rulesopt, opts, optimize,
 		    anchorname, NULL))
 			error = 1;
-		else if (!(opts & PF_OPT_NOACTION))
-			warn_namespace_collision(NULL);
 	}
 
 	if (opts & PF_OPT_ENABLE)
diff --git a/sbin/pfctl/pfctl.h b/sbin/pfctl/pfctl.h
index 776c106d28b..7981cf66fdb 100644
--- a/sbin/pfctl/pfctl.h
+++ b/sbin/pfctl/pfctl.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl.h,v 1.59 2019/01/02 23:08:00 kn Exp $ */
+/*	$OpenBSD: pfctl.h,v 1.60 2019/01/11 01:56:54 kn Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -79,7 +79,7 @@ void	 pfctl_clear_tables(const char *, int);
 void	 pfctl_show_tables(const char *, int);
 int	 pfctl_table(int, char *[], char *, const char *, char *,
 	    const char *, int);
-void	 warn_namespace_collision(const char *);
+void	 warn_duplicate_tables(const char *, const char *);
 void	 pfctl_show_ifaces(const char *, int);
 FILE	*pfctl_fopen(const char *, const char *);
 
diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c
index 7adad9da627..6ed4024da4e 100644
--- a/sbin/pfctl/pfctl_table.c
+++ b/sbin/pfctl/pfctl_table.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_table.c,v 1.79 2019/01/02 23:08:00 kn Exp $ */
+/*	$OpenBSD: pfctl_table.c,v 1.80 2019/01/11 01:56:54 kn Exp $ */
 
 /*
  * Copyright (c) 2002 Cedric Berger
@@ -92,7 +92,8 @@ static const char	*istats_text[2][2][2] = {
 			goto _error;					\
 		}							\
 		if (nadd) {						\
-			warn_namespace_collision(table.pfrt_name);	\
+			warn_duplicate_tables(table.pfrt_name,		\
+			    table.pfrt_anchor);				\
 			xprintf(opts, "%d table created", nadd);	\
 			if (opts & PF_OPT_NOACTION)			\
 				return (0);				\
@@ -526,12 +527,10 @@ pfctl_define_table(char *name, int flags, int addrs, const char *anchor,
 }
 
 void
-warn_namespace_collision(const char *filter)
+warn_duplicate_tables(const char *tablename, const char *anchorname)
 {
 	struct pfr_buffer b;
 	struct pfr_table *t;
-	const char *name = NULL, *lastcoll;
-	int coll = 0;
 
 	bzero(&b, sizeof(b));
 	b.pfrb_type = PFRB_TABLES;
@@ -547,22 +546,13 @@ warn_namespace_collision(const char *filter)
 	PFRB_FOREACH(t, &b) {
 		if (!(t->pfrt_flags & PFR_TFLAG_ACTIVE))
 			continue;
-		if (filter != NULL && strcmp(filter, t->pfrt_name))
+		if (!strcmp(anchorname, t->pfrt_anchor))
 			continue;
-		if (!t->pfrt_anchor[0])
-			name = t->pfrt_name;
-		else if (name != NULL && !strcmp(name, t->pfrt_name)) {
-			coll++;
-			lastcoll = name;
-			name = NULL;
-		}
+		if (!strcmp(tablename, t->pfrt_name))
+			warnx("warning: table <%s> already defined"
+			    " in anchor \"%s\"", tablename,
+			    t->pfrt_anchor[0] ? t->pfrt_anchor : "/");
 	}
-	if (coll == 1)
-		warnx("warning: namespace collision with <%s> global table.",
-		    lastcoll);
-	else if (coll > 1)
-		warnx("warning: namespace collisions with %d global tables.",
-		    coll);
 	pfr_buf_clear(&b);
 }