Re: sockaddr_un.sun_len vs. reality

On Tue, Feb 15, 2022 at 4:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > On Mon, Feb 14, 2022 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I'm leaning to adding a compile-time clamp on the value,
> >> that is
> >>
> >>      unp->sun_len = Min(sizeof(struct sockaddr_un),
> >>                         (1 << (sizeof(unp->sun_len) * 8)) - 1);
>
> > Any system that has sun_len, and has expanded sun_path so that the
> > struct size doesn't fit in sun_len, must surely be ignoring sun_len
> > but retaining it for binary compatibility.  Otherwise there would be
> > no way to use the extra bytes of sun_path!  It's tempting to suggest
> > setting sun_len to zero in this case...
>
> Yeah, I thought about doing that or just skipping the assignment
> altogether.  However, we'd need just as much code, because the
> change would have to be conditional on more or less this same
> computation as to whether sizeof(struct sockaddr_un) fits into
> the field.

I was nerd-sniped by the historical context of this single line of
code.  I'd already wondered many times (not just in PostgreSQL)
whether and when that became a cargo-cult practice, replicated from
other software and older books like Stevens.  I failed to find any
sign of an OS that needs it today, or likely even needed it this
millennium.  Now I'd like to propose removing it.

I believe we have the complete set of surviving systems with sun_len.
These are the systems with sockets descended from 4.4BSD, plus AIX
when using its socket API in 4.4-compatible mode:

AIX: no sun_len if -DCOMPAT_43[1], so surely ignored by kernel!
NetBSD: manual says it's ignored[2]
OpenBSD: ditto[3]
FreeBSD: ditto[4]
DragonFlyBSD: clobbered[5] (like FreeBSD, just not documented)
macOS: ditto[6]

I know that between '88 and '97 some of these would fail with EINVAL
if sun_len was out of range.  The code is still there to do that in
some cases, but at various points in the 90s they started clobbering
it on entry to connect() and bind(), probably to ease porting pain
from Solaris and Linux.  I think it'd be pretty clear on the build
farm if it turns out I'm wrong here, because the zero-initialised
sun_len would be rejected as invalid on a hypothetical system that
didn't change that policy.  I've tested on all but AIX.

[1] https://www.postgresql.org/message-id/IKEPJJEJDCJPKMLEECEDGEIHCCAA.vvanwynsberghe%40ccncsi.net
[2] https://man.netbsd.org/unix.4
[3] https://man.openbsd.org/connect.2
[4] https://www.freebsd.org/cgi/man.cgi?connect
[5] https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/kern/uipc_syscalls.c#L1516
[6] https://github.com/apple/darwin-xnu/blob/main/bsd/kern/uipc_syscalls.c#L2817