Re: sockaddr_un.sun_len vs. reality

On Mon, Feb 14, 2022 at 7:19 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> For a very long time, the AIX 7.1 buildfarm members (sungazer and tern)
> have complained about
>
> ip.c:236:17: warning: conversion from 'long unsigned int' to 'uchar_t' {aka 'unsigned char'} changes value from '1025' to '1' [-Woverflow]
>
> I'd ignored this as not being very interesting, but I got motivated to
> recheck it today. The warning is coming from
>
> #ifdef HAVE_STRUCT_SOCKADDR_STORAGE_SS_LEN
> unp->sun_len = sizeof(struct sockaddr_un);
> #endif
>
> so we can infer that the sun_len field is unsigned char and the
> size of struct sockaddr_un doesn't fit.

Yeah, it's the GCC AIX animals. I wondered if xlc might be seeing
different structs or something but no, I tried on an AIX machine and
it just doesn't warn about that.

> Poking around a bit, I find that sun_len exists on most BSD-ish
> platforms and it seems to be unsigned char (almost?) everywhere.
> But on other platforms sizeof(struct sockaddr_un) is only a bit
> over 100, so there's not an overflow problem.
>
> It's not real clear that there's any problem on AIX either;
> given that the regression tests pass, I strongly suspect that
> nothing is paying attention to the sun_len field. But if we're
> going to fill it in at all, we should probably try to fill it
> in with something less misleading than "1".
>
> Googling finds that this seems to be a sore spot for various
> people, eg [1] mentions the existence of the SUN_LEN() macro
> on some platforms and then says why you shouldn't use it.
>
> 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);
>
> (This might need a little bit of refinement if sun_len
> could be as wide as int, but in any case it should still
> reduce to a compile-time constant.)
>
> Or we could use something involving strlen(unp->sun_path), perhaps
> trying to use SUN_LEN() if it exists. But that implies expending
> extra run-time cycles for strlen(), and I've seen no indication
> that there's any value here except suppressing a compiler warning.
>
> Thoughts?

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...

Huh, apparently AIX expanded sun_path in 5.3, also creating a
contradiction with sockaddr_storage and crashing PostgreSQL[1].

[1] https://www.postgresql.org/docs/8.4/installation-platform-notes.html