As discussed in private, ok gilles@ with this diff in 7.5, just for the record


March 1, 2024 10:37 PM, "Omar Polo" <op@omarpolo.com> wrote:

> friendly ping, anyone has an opinion?
> 
> Omar Polo <op@omarpolo.com> wrote:
> 
>> 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;