[issue12837] Patch for issue #12810 removed a valid check on socket ancillary data

[David Watson]

David Watson <baikie at users.sourceforge.net> added the comment:

On Wed 24 Aug 2011, Charles-François Natali wrote:
> > I included this test deliberately, because msg_controllen may be 
> > of signed type [...] POSIX allows socklen_t to be signed
> 
> http://pubs.opengroup.org/onlinepubs/007908799/xns/syssocket.h.html
> """
> <sys/socket.h> makes available a type, socklen_t, which is an unsigned opaque integral type of length of at least 32 bits. To forestall portability problems, it is recommended that applications should not use values larger than 2**32 - 1.
> """

That has since been changed.  I'm reading from POSIX.1-2008,
which says:

   The <sys/socket.h> header shall define the socklen_t type,
   which is an integer type of width of at least 32 bits

http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_socket.h.html

The warning against using values larger than 2**32 - 1 is still
there, I presume because they would not fit in a 32-bit signed
int.

> Also, I'm not convinced by this:
> 
>    /* Check for empty ancillary data as old CMSG_FIRSTHDR()
>        implementations didn't do so. */
>     for (cmsgh = ((msg.msg_controllen > 0) ? CMSG_FIRSTHDR(&msg) : NULL);
>          cmsgh != NULL; cmsgh = CMSG_NXTHDR(&msg, cmsgh)) {
> 
> Did you really have reports of CMSG_NXTHDR not returning NULL upon empty ancillary data (it's also raquired by POSIX)?

I take it you mean CMSG_FIRSTHDR here; RFC 3542 says that:

   One possible implementation could be

      #define CMSG_FIRSTHDR(mhdr) \
          ( (mhdr)->msg_controllen >= sizeof(struct cmsghdr) ? \
            (struct cmsghdr *)(mhdr)->msg_control : \
            (struct cmsghdr *)NULL )

   (Note: Most existing implementations do not test the value of
   msg_controllen, and just return the value of msg_control...

IIRC, I saw an implementation in old FreeBSD headers that did not
check msg_controllen, and hence did not return NULL as RFC 3542
requires.

Now you come to mention it though, this check in the for loop
does actually protect against the kernel returning a negative
msg_controllen, so the only remaining possibility would be that
the CMSG_* macros fiddle with it.

That said, the fact remains that the compiler warning is spurious
if msg_controllen can be signed on some systems, and I still
don't think decreasing the robustness of the code (particularly
against any future modifications to that code) just for the sake
of silencing a spurious warning is a good thing to do.  People
can read the comment above the "offending" line and see that the
compiler has got it wrong.

----------

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue12837>
_______________________________________