sockaddr_un.sun_len vs. reality - Mailing list pgsql-hackers

From Tom Lane
Subject sockaddr_un.sun_len vs. reality
Date
Msg-id 2781112.1644819528@sss.pgh.pa.us
Whole thread Raw
Responses Re: sockaddr_un.sun_len vs. reality
Re: sockaddr_un.sun_len vs. reality
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Kyotaro Horiguchi
Date:
Subject: Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
Next
From: Tom Lane
Date:
Subject: Re: How did queensnake corrupt zic.o?