On Tue Feb 20, 2024 at 10:59 PM CET, Gilles Chehade wrote:
> On Tue, Feb 20, 2024 at 05:10:52PM +0100, Omar Polo wrote:
> > [...]
> > So, I'm proposing to require ipv6 addresses to be wrapped in braces in
> > table(5) files, and to amend the parser to skip over to the matching ]
> > if the line starts with [.  Is it enough or should it be more strict?
> > 
>
> I dunno if it should be more strict but that would already be a good
> start and an improvement over today.

One downside I realized only later is that it still allows gibberish
after the ip addr, as in "[::1]foo".

> > [...]
> > 
> > thoughs?
> > 
>
> Generally ok with this but also starting to wonder if makemap and static
> tables parsing would not benefit from a regex(3)-based line parser to be
> honest. It's not used anywhere an expensive regex would be an issue, and
> it would be more maintainable than having to read it every ten year when
> someone hits a case we didn't hit before.
>
> Dunno, just opening the discussion

Not sure if a regexp is needed here, but for sure it could be
interesting to eventually settle on some other details of the parser.
For example, I these confusing:

	foo :: bar	becomes		"foo" -> ":: bar"
	foo::bar	becomes		"foo" -> ":bar"
	foo:: bar	becomes		"foo" -> "bar"

so, some details should be agreed upon regardless of the implementation
of the parser.

my diff also introduces another ambiguity: what to do when there
is no closing ']'?  does it makes sense for keys to include '['
outside of ipv6 addresses?  (I guess no, but don't know for sure.)

Here's another try, where I may have gone too far, which considers
a parse error if there is no closing ] and also improves IMHO the
splitting of key-value.  Using the same example table as before,
now it becomes:

	suzaku% cat t
	[::1]: bar
	[::2]   bar
	foo1    bar
	foo2:   bar
	foo3:bar
	foo4 : bar
	foo5 :: bar
	foo6::bar
	foo7:: bar
	suzaku% ./makemap t && ./makemap -U t.db | sort
	[::1]   bar
	[::2]   bar
	foo1    bar
	foo2    bar
	foo3    bar
	foo4    : bar
	foo5    :: bar
	foo6    :bar
	foo7    : bar

i.e. it splits on the first space or colon, and then trims the
spaces.  Multiple colons are not trimmed.  Does anybody have aliases
lines with "foo : bar"?


diffstat /home/op/w/smtpd
 M  table.5  |  10+   7-
 M  util.c   |  15+  23-

2 files changed, 25 insertions(+), 30 deletions(-)

diff /home/op/w/smtpd
commit - 0e1879aafc8d52781267651a6455236cadaf2b54
path + /home/op/w/smtpd
blob - 65dca359f38d257db5d8c948374a8433c15e1a9d
file + table.5
--- table.5
+++ table.5
@@ -173,10 +173,11 @@ of addresses in the table until a match is found.
 A netaddr table can contain exact addresses or netmasks, and looks as follow:
 .Bd -literal -offset indent
 192.168.1.1
-::1
-ipv6:::1
+[::1]
 192.168.1.0/24
 .Ed
+.Pp
+IPv6 addresses must be enclosed in square brackets.
 .Ss Userinfo tables
 Userinfo tables are used in rule context to specify an alternate userbase,
 mapping virtual users to local system users by UID, GID and home directory.
@@ -214,11 +215,11 @@ A source table looks as follow:
 .Bd -literal -offset indent
 192.168.1.2
 192.168.1.3
-::1
-::2
-ipv6:::3
-ipv6:::4
+[::1]
+[::2]
 .Ed
+.Pp
+IPv6 address must be enclosed in square brackets.
 .Ss Mailaddr tables
 Mailaddr tables are lists of email addresses.
 They can be used in the following contexts:
@@ -254,10 +255,12 @@ outgoing connection.
 .Pp
 The format is a mapping from inet4 or inet6 addresses to hostnames:
 .Bd -literal -offset indent
-::1		localhost
+[::1]		localhost
 127.0.0.1	localhost
 88.190.23.165	www.opensmtpd.org
 .Ed
+.Pp
+IPv6 addresses must be enclosed in square brackets.
 .Sh SEE ALSO
 .Xr smtpd.conf 5 ,
 .Xr makemap 8 ,
blob - d481162c8973a288e6470ba1ef1367ad487b8d2d
file + util.c
--- util.c
+++ util.c
@@ -855,7 +855,7 @@ int
 parse_table_line(FILE *fp, char **line, size_t *linesize,
     int *type, char **key, char **val, int *malformed)
 {
-	char	*keyp, *valp, *p;
+	char	*keyp, *valp;
 	ssize_t	 linelen;
 
 	*key = NULL;
@@ -885,37 +885,29 @@ parse_table_line(FILE *fp, char **line, size_t *linesi
 		return 0;
 	}
 
-	if (*type == T_NONE) {
-		for (p = keyp; *p; p++) {
-			if (*p == ' ' || *p == '\t' || *p == ':') {
-				*type = T_HASH;
-				break;
-			}
+	if (*keyp == '[') {
+		if ((valp = strchr(keyp, ']')) == NULL) {
+			*malformed = 1;
+			return (0);
 		}
-		if (*type == T_NONE)
-			*type = T_LIST;
-	}
+		valp++;
+	} else
+		valp = keyp + strcspn(keyp, " \t:");
 
+	if (*type == T_NONE)
+		*type = (*valp == '\0') ? T_LIST : T_HASH;
+
 	if (*type == T_LIST) {
 		*key = keyp;
 		return (0);
 	}
 
 	/* T_HASH */
-	valp = keyp;
-	strsep(&valp, " \t:");
-	if (valp) {
-		while (*valp) {
-			if (!isspace((unsigned char)*valp) &&
-			    !(*valp == ':' &&
-			    isspace((unsigned char)*(valp + 1))))
-				break;
-			++valp;
-		}
-		if (*valp == '\0')
-			valp = NULL;
+	if (*valp != '\0') {
+		*valp++ = '\0';
+		valp += strspn(valp, " \t");
 	}
-	if (valp == NULL)
+	if (*valp == '\0')
 		*malformed = 1;
 
 	*key = keyp;

-- 
Sent from