Re: sockaddr_un.sun_len vs. reality
On Tue, Aug 23, 2022 at 3:43 PM Tom Lane wrote: > Thomas Munro writes: > > 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. > > Seems worth a try. Pushed, and build farm looks good. For the benefit of anyone else researching this topic, I should add that Stevens in fact said it's OK to skip this, and if I had opened UNIX Network Programming (3rd ed) volume I to page 99 I could have saved myself some time: "Even if the length field is present, we need never set it and need never examine it, unless we are dealing with routing sockets ...".
Re: sockaddr_un.sun_len vs. reality
Thomas Munro writes: > 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. Seems worth a try. regards, tom lane
Re: sockaddr_un.sun_len vs. reality
On Tue, Feb 15, 2022 at 4:21 AM Tom Lane wrote: > Thomas Munro writes: > > On Mon, Feb 14, 2022 at 7:19 PM Tom Lane 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 From 0a7c7e8c288300f15cea0477d5d692f652f5f44b Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 23 Aug 2022 13:16:54 +1200 Subject: [PATCH] Don't bother to set sockaddr_un.sun_len. There are no known systems that require you to fill in sun_len when calling bind() or connect(). We might as well stop perpetuating this obsolete practice. Discussion: https://postgr.es/m/2781112.1644819528%40sss.pgh.pa.us --- src/common/ip.c | 14 -- 1 file changed, 14 deletions(-) diff --git a/src/common/ip.c b/src/common/ip.c index 9b611cdc8c..6343f49a70 100644 --- a/src/common/ip.c +++ b/src/common/ip.c @@ -218,20 +218,6 @@ getaddrinfo_unix(const char *path, const struct addrinfo *hintsp, aip->ai_addrlen = offsetof(struct sockaddr_un, sun_path) + strlen(path); } - /* - * The standard recommendation for filling sun_len is to set it to the - * struct size (independently of the actual path length). However, that - * draws an integer-overflow warning on AIX 7.1, where sun_len is just - * uint8 yet the struct size exceeds 255 bytes. It's likely that nothing - * is paying attention to sun_len on that platform, but we have to do - * something with it. To suppress the warning, clamp the struct size to - * what will fit in sun_len. - */ -#ifdef HAVE_STRUCT_SOCKADDR_SA_LEN - unp->sun_len = Min(sizeof(struct sockaddr_un), - ((size_t) 1 << (sizeof(unp->sun_len) * BITS_PER_BYTE)) - 1); -#endif - return 0; } -- 2.35.1
Re: sockaddr_un.sun_len vs. reality
On Tue, Feb 15, 2022 at 3:25 AM Robert Haas wrote: > It's not real clear to me that it's worth complicating the code to > avoid a harmless compiler warning on an 11-year-old operating system > with minimal real-world usage. On the other hand, if you really feel > motivated to do something about it, I'm not here to argue. My one > suggestion is that if you do add some strange incantation here along > the lines you propose, you should at least add a comment explaining > that it was done to suppress a warning on AIX 7.1. That way, there's a > chance someone might be brave enough to try removing it in the future > at such time as nobody cares about AIX 7.1 any more. Just for the record, it's not just 11-year-old 7.1; 7.2 contains this contradiction too, but our 7.2 animal doesn't complain because it's using a different compiler. I bet you one internet point 7.3 (which just dropped in December, but isn't yet available for open source hackers to scrounge an account on in the GCC farm) is the same. By the way, speaking of open source on AIX, this distribution is coming to an end: http://www.bullfreeware.com/newsPage
Re: sockaddr_un.sun_len vs. reality
Thomas Munro writes: > On Mon, Feb 14, 2022 at 7:19 PM Tom Lane 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. regards, tom lane
Re: sockaddr_un.sun_len vs. reality
Robert Haas writes:
> On Mon, Feb 14, 2022 at 1:19 AM Tom Lane 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]
> It's not real clear to me that it's worth complicating the code to
> avoid a harmless compiler warning on an 11-year-old operating system
> with minimal real-world usage. On the other hand, if you really feel
> motivated to do something about it, I'm not here to argue.
Basically, yesterday's discussion motivated me to try to clean up some
of the stray buildfarm warnings I've been ignoring for a long time.
I agree it doesn't look like this change would have any real-world
impact. But we do set some value on warning-free builds, if only to
save ourselves having to remember "this warning is harmless".
> My one
> suggestion is that if you do add some strange incantation here along
> the lines you propose, you should at least add a comment explaining
> that it was done to suppress a warning on AIX 7.1.
But of course.
regards, tom lane
Re: sockaddr_un.sun_len vs. reality
On Mon, Feb 14, 2022 at 1:19 AM Tom Lane 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.
>
> 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".
It's not real clear to me that it's worth complicating the code to
avoid a harmless compiler warning on an 11-year-old operating system
with minimal real-world usage. On the other hand, if you really feel
motivated to do something about it, I'm not here to argue. My one
suggestion is that if you do add some strange incantation here along
the lines you propose, you should at least add a comment explaining
that it was done to suppress a warning on AIX 7.1. That way, there's a
chance someone might be brave enough to try removing it in the future
at such time as nobody cares about AIX 7.1 any more.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: sockaddr_un.sun_len vs. reality
On Mon, Feb 14, 2022 at 7:19 PM Tom Lane 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
sockaddr_un.sun_len vs. reality
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.
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?
regards, tom lane
[1] http://mail-index.netbsd.org/tech-net/2006/10/11/0008.html
