ipfw: Support [w:x:y::z]:port (bracketed) IPv6 addresses in the fwd command

[nc]

Summary

ipfw: Support {w:x:y::z}:port (bracketed) IPv6 addresses in the fwd command.

While we're doing this, optimize by not attempting to parse addresses if we're using tableargs.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

Test Plan

Try compiling HEAD with this patch (or at least /usr/src/sbin/ipfw) and then run the following command:

ipfw add fwd "[::1]:3128" tcp from any to any

It should work

[donner]

How about detecting the port separator first? (i.e. repeatly call strpbrk)
Then you can easily distinguish between the cases

• starts with '[' -> numeric IPb6
• contains ':' -> numeric IPv6
• contains no letters -> numeric IPv4
• use gai()

[donner]

In D24011#527998, @lutz_donnerhacke.de wrote:

How about detecting the port separator first? (i.e. repeatly call strpbrk)

getaddrinfo let you simplify this even more:

• strchr(',') and if found, you have the addr/port separated.
• otherwise strrchr(':') and if found, separation done
• if first char '[', check and remove last char ']', set numeric flag
• call getaddrinfo with both pieces

Do not try to parse things, which are already handled by gai.

[nc]

Your IPv4 suggestion is wrong. There can be IPv6 addresses without letters, like 2001:470:20::2, or 2600::1.

Also, your suggestions would require rewriting of the IP parsing code I modify, so I'd prefer not to do this unless necessary.

However, it's possible with my code that you can do IPv4 in brackets, like [1.2.3.4]:56789. I don't believe this causes any harm, but if it does I'm open to modifications or even the "rewrite" I mentioned earlier.

[donner]

That would be my approach https://reviews.freebsd.org/differential/diff/69565/
I'd further eliminate the temporary storage "struct sockaddr_storage result", and copy directly from the gai result into the action (with memcpy).

[nc]

Your approach looks good, reducing memory consumption.

[rgrimes]

Adding bz, as it is his XXX-BZ comment being removed about implementing this.

[rgrimes]

sbin/ipfw/ipfw2.c

4042 ↗

(On Diff #69346)

So technically I can not have a hostnamed tablearg?

4046 ↗

(On Diff #69346)

I would think all of the above attempt at parsing an IP address belongs inside this else block as none of it is needed in the first half of this if.

4051 ↗

(On Diff #69346)

Magic pointer arithmetic? v6brac is 0 or 1 when we get here, so probably some place an *av++ should of happened to indicate we have swallowed a token?

[nc]

Here, I resolve two of three of rgrimes' suggestions.

About the hostnamed tablearg, I believe it should stay the same (CLARIFICATION: as it is today) but did not test this.

[rgrimes]

That reads much better, cleaned it up a fair bit more than I thought it would.

[nc]

I'm not a committer (I'm also not neel@), could someone please commit this?

[markj]

ipfw2.c

3992

Hmm, is this standard syntax for IPv4 addresses? I have never seen it.

4040

Per style(9), casts are not followed by a space.

[nc]

Made the corrections.

[donner]

ipfw2.c

3992

No, but a canonical extension.

[markj]

This looks ok to me modulo the nits I pointed out.

sbin/ipfw/ipfw2.c

3999 ↗

(On Diff #73587)

style(9) discourages combined declarations and initializations.

4019 ↗

(On Diff #73587)

Similarly here. ptr also doesn't seem like a great name to me. I think you could simply do without this variable:

s = strchr(*av, ']');
if (s != NULL) {
    *(s++) = '\0';
} else {
    s = *av;
}
s = strchr(s, ':');
...

[nc]

Good catch on the tidbits, here's an revised patch.