netinet: enforce broadcast mode for all-ones and all-zeroes destinations

[glebius]

Summary

When a socket has SO_BROADCAST set and destination address is INADDR_ANY
or INADDR_BROADCAST, the kernel shall pick up first broadcast capable
interface and broadcast the packet out of it. Since this API is not
reliable on a machine with > 1 broadcast capable interfaces, all practical
software seems to use IP_ONESBCAST or other mechanisms to send broadcasts.
This has been broken at least since FreeBSD 6.0, see bug 99558. Back then
the problem was in the fact that in_broadcast() check was always done
against the gateway address, not the destination address. Later, with
90cc51a1ab4be, a second problem piled on top - we aren't checking for
INADDR_ANY and INADDR_BROADCAST at all.

Better late than never, fix that by checking destination address.

PR: 99558

[markj]

sys/netinet/in.h

690

Why add it to the header? Is there a follow-up patch which adds another consumer?

692

This is fine because these addresses have the same representation in both host and network order, but formally we should check ntohl(in.s_addr) instead.

[glebius]

sys/netinet/in.h

690

I don't have a patch/review yet, but indeed there are few places which have exactly this check unrolled.

692

I'd rather add htonl() around the constants. Will do.

[glebius]

Added htonl()s.

[zlei]

sys/netinet/in.h

692

I'd rather add htonl() around the constants. Will do.

INADDR_BROADCAST is all 1s, and INADDR_ANY is all 0s. So htonl() does not make any differences but it will be even worse that, when a developer read this he may wonder if INADDR_BROADCAST or INADDR_ANY may change in future.

It also is not consistent with existing usage, for example the following,

#define	in_nullhost(x)	((x).s_addr == INADDR_ANY)

Normally we check ntohl(in.s_addr), but since this htonl(INADDR_ANY) is a sub-optimizition, why not step further and remove pointless htonl() ? If that makes any obfuscation I think a comment explain why no htonl() is fair enough.

[zlei]

sys/netinet/in.h

692

Also, INADDR_BROADCAST and INADDR_ANY has type in_addr_t ,

#define	INADDR_ANY		((in_addr_t)0x00000000)
#define	INADDR_BROADCAST	((in_addr_t)0xffffffff)	/* must be masked */

So I'd say it is perfect to compare in.s_addr with INADDR_ANY and INADDR_BROADCAST directly.

[glebius]

Even though the constants aren't going to change, the whole point of having constants instead of bare values is to hide the values from developer. A code that relies on particular value of a constant isn't technically correct, even though constant is very unlikely to change. We could argue on using technically incorrect, but practically working code if some performance or other side effect was at stake. But here there is nothing at stake. ntohl on constant will be compiled to nothing with the compiler.

[glebius]

P.S. in_nullhost() can be improved, rather than used as an example.