classification
Title: Patch for issue #12810 removed a valid check on socket ancillary data
Type: behavior Stage: commit review
Components: Extension Modules Versions: Python 3.3, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: baikie, brett.cannon, christian.heimes, ncoghlan, neologix, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2011-08-24 21:56 by baikie, last changed 2014-01-07 22:03 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
restore_controllen_check.diff baikie, 2011-08-24 21:56 Restore check for msg->msg_controllen < 0 in cmsg_min_space() review
socket_compiler_warning.diff brett.cannon, 2011-09-01 05:06 review
pragma.diff brett.cannon, 2013-10-18 14:31 Use pragmas to turn off warning review
socket-compare-cmsg_len_end.diff baikie, 2013-12-05 17:22 review
silence_compiler_warning.diff brett.cannon, 2013-12-13 16:55 review
Messages (25)
msg142934 - (view) Author: David Watson (baikie) Date: 2011-08-24 21:56
Changeset 4736e172fa61 for issue #12810 removed the test 
"msg->msg_controllen < 0" from socketmodule.c, where 
msg_controllen happened to be unsigned on the reporter's system. 
 
I included this test deliberately, because msg_controllen may be 
of signed type (POSIX allows socklen_t to be signed, as objects 
of that type historically were - as the Rationale says: "All 
socklen_t types were originally (in BSD UNIX) of type int."). 
 
Attaching a patch to replace the check and add an accompanying 
comment.
msg142935 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-24 22:13
> 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.
"""

It seems pretty clear to me.
Did you actually encounter this problem on any OS?
Furthermore, even if it was the case, I don't see how we could end up with a negative value for msg_controllen (it's a buffer length), since it's set by the kernel.
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)?
msg142936 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-24 22:35
I checked in glibc, FreeBSD and OpenBSD source codes, and they all define socklen_t as an unsigned integer.
I think the confusion arises from this:
"""
       The  third argument of accept() was originally declared as an int * (and is that under libc4 and libc5 and on many other systems like 4.x BSD, SunOS 4, SGI); a POSIX.1g
       draft standard wanted to change it into a size_t *, and that is what it is for SunOS 5.  Later POSIX drafts have socklen_t *, and so do the  Single  Unix  Specification
       and glibc2.
"""

But this only implies that sizeof(socklen_t) == sizeof(int).

> since it's set by the kernel.

The only place where we compute it is in sock_sendmsg, but it would be catched by overflow checks:
        if (controllen > SOCKLEN_T_LIMIT || controllen < controllen_last) {
            PyErr_SetString(socket_error, "too much ancillary data");
            goto finally;
        }

And we use it as malloc() and memset() argument before it's checked by cmsg_min_space...
msg142992 - (view) Author: David Watson (baikie) Date: 2011-08-25 21:09
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.
msg143099 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-28 15:56
> That has since been changed.  I'm reading from POSIX.1-2008,
> which says:

I see.

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

I assume you mean 2**31 - 1.

> I take it you mean CMSG_FIRSTHDR here

Indeed.

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

Alright, that's all I wanted to know.

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

Well, the compiler does not get it wrong. If socklen_t is defined as
an unsigned int, it has no way of knowing that it might be defined as
signed int on other platforms.
msg143102 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-28 16:22
New changeset 3ed2d087e70d by Charles-François Natali in branch 'default':
Issue #12837: POSIX.1-2008 allows socklen_t to be a signed integer: re-enable
http://hg.python.org/cpython/rev/3ed2d087e70d
msg143103 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-08-28 16:27
Thanks for the patch.

For the record, here's Linus Torvalds' opinion on this whole socklen_t confusion:
"""
_Any_ sane library _must_ have "socklen_t" be the same size as int.  Anything else breaks any BSD socket layer stuff.  POSIX initially did make it a size_t, and I (and
       hopefully others, but obviously not too many) complained to them very loudly indeed.  Making it a size_t is completely broken, exactly because size_t very seldom is the
       same  size  as  "int" on 64-bit architectures, for example.  And it has to be the same size as "int" because that's what the BSD socket interface is.  Anyway, the POSIX
       people eventually got a clue, and created "socklen_t".  They shouldn't have touched it in the first place, but once they did they felt it had to have a named  type  for
       some unfathomable reason (probably somebody didn't like losing face over having done the original stupid thing, so they silently just renamed their blunder).
"""
msg143298 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-09-01 05:06
This is now generating a compiler warning under OS X because the older POSIX standard is followed where socklen_t can be unsigned.

Attached is a patch to cast msg_controllen to a size big enough to hold either signed 2**31-1 or unsigned 2**32-1 (i.e., long long) to silence the compiler warning. I would check the patch in myself but test_socket is failing under OS X right now so I can't verify I didn't do something extremely stupid with the cast somehow.
msg143307 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-09-01 06:37
`long long` is not ANSI, but C99.
Anyhow, I'm still not sure this check is necessary, because:
1) I really doubt any modern OS uses a signed socklen_t
2) even if it's the case, I don't see how we could possibly end up with a negative msg_controllen

I'm adding Nick and Antoine to the noisy list to know what they think about this...
msg143309 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-09-01 07:23
I tend to be fairly paranoid about operating systems doing occasionally bizarre things, so I like having clearly defined behaviour even when the OS does something arguably nuts, but permitted by the relevant standard. Hence I think keeping the check is the right thing to do, which means finding an alternative to silence the compiler warning on Mac OS X (and similar systems).

The "right" way is probably a configure check, but that feels like overkill for something minor like this.

Brett's patch seems like a reasonable alternative if an appropriate guaranteed-to-be-at-least-64-bits type can be found, but 'long long' isn't a valid cast target for this unless Guido is OK with PEP 7 being modified to include a small number of permitted C99 features.
msg143319 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-09-01 12:07
If you're casting to a larger signed type, then the semantics change, since there is a sign extension.
For example (unsigned int) 0xFFFFFFFF could be cast to (long long) -1.

You could cast to size_t instead and compare the result to SOCKLEN_T_MAX (which currently doesn't exist :-)).
msg153746 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-02-20 00:55
Since we never solved this I'm still getting compiler warnings. Can we decide on a solution for this so I can go back to be warning-free (sans `-Wno-unused-value -Wno-empty-body -Qunused-arguments`)?


/Users/bcannon/Developer/repo/cpython/bootstrap_importlib/Modules/socketmodule.c:1439:22: warning: comparison of unsigned expression < 0 is always false
      [-Wtautological-compare]
        if (flowinfo < 0 || flowinfo > 0xfffff) {
            ~~~~~~~~ ^ ~
/Users/bcannon/Developer/repo/cpython/bootstrap_importlib/Modules/socketmodule.c:1948:74: warning: comparison of unsigned expression < 0 is always false
      [-Wtautological-compare]
    if (cmsgh == NULL || msg->msg_control == NULL || msg->msg_controllen < 0)
                                                     ~~~~~~~~~~~~~~~~~~~ ^ ~
/Users/bcannon/Developer/repo/cpython/bootstrap_importlib/Modules/socketmodule.c:5062:18: warning: comparison of unsigned expression < 0 is always false
      [-Wtautological-compare]
    if (flowinfo < 0 || flowinfo > 0xfffff) {
        ~~~~~~~~ ^ ~
3 warnings generated.
msg194974 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-08-12 17:48
This is still a warning and so I'm still looking for a solution.
msg200262 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-10-18 14:31
Attached is a patch that silences the warning for just the 'if' statement under Clang using pragmas. I don't know if we have ever had it out on python-dev on our view of using pragmas, but this seems like a reasonable place to use it.
msg205205 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-12-04 08:09
I'd like to see the warning silenced before 3.4 gets released, too. How about a check like

   (Py_ssize_t)msg->msg_controllen > 0xffffffffL

instead? I'd also be fine with pragmas.
msg205268 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-05 00:34
Is there any point in littering the source code to avoid platform-specific warnings? The difference is probably that some fields are defined unsigned on OS X while they're signed on other platforms.

(also, your patch has lots of unrelated changes)
msg205278 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-12-05 06:35
I agree with Antoine.
Silencing warning is fine, as long as it's not to the detriment of
clarity (see Debian Openssl's vulnerability extreme example).
msg205302 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-12-05 14:40
First, sorry about the noise in the patch.

Second, the patch should contain just three pragma lines:

  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wtautological-compare"
  ...
  #pragma clang diagnostic pop

Now I don't think that's unreadable at all, nor runs any risk of breaking anything. And since the majority of Clang users are probably on OS X that would suggest that the suppression of the warning will only affect those where the warning is superfluous and won't lead to unneeded suppressions (if people are really worried about unneeded suppressions I can try to break the 'if' guard up to only suppress on the one part).

I just don't want to perpetually be ignoring a single warning in the code. That leads to me ignoring any warnings that come during compilation, which is unfortunate as some warnings are actually useful and should be rectified.
msg205308 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-05 15:43
On jeu., 2013-12-05 at 14:40 +0000, Brett Cannon wrote:
> 
> I just don't want to perpetually be ignoring a single warning in the
> code. That leads to me ignoring any warnings that come during
> compilation, which is unfortunate as some warnings are actually useful
> and should be rectified.

I guess my annoyance is that people reading the code will think "what is
this here for?". Perhaps you could add a comment.
msg205312 - (view) Author: David Watson (baikie) Date: 2013-12-05 17:22
Looking again at cmsg_min_space(), I see that it already returns
false when msg_controllen is less than cmsg_len_end, so you could
do a (signed) comparison against that, rather than 0.  Patch
attached.
msg206119 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-12-13 16:55
Two things. First, I'm sorry David but my mind is not working fully enough at the moment to see how msg_controllen is compared to cmsg_len_end without relying on external value coming in through the parameters of the function.

Second, I have re-uploaded my patch using pragmas to silence the comparison so it's easier to read.
msg206132 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-12-13 18:03
Christian Heimes pointed out #ifdef __clang__ might be necessary to silence warnings on other platforms.
msg206266 - (view) Author: David Watson (baikie) Date: 2013-12-15 23:12
On Fri 13 Dec 2013, Brett Cannon wrote:
> Two things. First, I'm sorry David but my mind is not working fully enough at the moment to see how msg_controllen is compared to cmsg_len_end without relying on external value coming in through the parameters of the function.

The lines (in the existing code)

    if (space < cmsg_len_end)
        space = cmsg_len_end;

ensure that space >= cmsg_len_end, and then we have

    return (cmsg_offset <= (size_t)-1 - space &&
            cmsg_offset + space <= msg->msg_controllen);

so that 0 is returned if msg->msg_controllen < (cmsg_offset +
space), but since cmsg_offset is nonnegative and cmsg_len_end <=
space, we always have cmsg_len_end <= (cmsg_offset + space).
Hence if we get to this last line and msg->msg_controllen <
cmsg_len_end, then msg->msg_controllen < (cmsg_offset + space),
and so the function returns 0.

(So returning 0 immediately if msg->msg_controllen < cmsg_len_end
doesn't change the behaviour of the function, provided this
comparison is done correctly.)
msg207606 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-07 22:01
New changeset 407c9c297ff7 by Brett Cannon in branch 'default':
Issue #12837: Silence a Clang compiler warning on OS X.
http://hg.python.org/cpython/rev/407c9c297ff7
msg207608 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-07 22:03
Ended up going with my solution as I just couldn't guarantee 100% that David's approach would always be right (for no fault of David's; at this point I just want a solution and so I'm willing to use compiler tricks to get it as I grok those).

Anyway, thanks to everyone for helping with this over the years.
History
Date User Action Args
2014-01-07 22:03:51brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg207608
2014-01-07 22:01:11python-devsetmessages: + msg207606
2013-12-15 23:12:41baikiesetmessages: + msg206266
2013-12-13 18:03:23brett.cannonsetmessages: + msg206132
2013-12-13 16:55:53brett.cannonsetfiles: + silence_compiler_warning.diff

messages: + msg206119
2013-12-05 17:22:46baikiesetfiles: + socket-compare-cmsg_len_end.diff

messages: + msg205312
2013-12-05 16:07:03brett.cannonsetassignee: brett.cannon
2013-12-05 15:43:28pitrousetmessages: + msg205308
2013-12-05 14:40:03brett.cannonsetmessages: + msg205302
2013-12-05 06:35:49neologixsetmessages: + msg205278
2013-12-05 00:34:28pitrousetmessages: + msg205268
2013-12-04 08:09:07christian.heimessetnosy: + christian.heimes

messages: + msg205205
versions: + Python 3.4
2013-10-18 14:31:17brett.cannonsetfiles: + pragma.diff

messages: + msg200262
2013-08-12 17:48:11brett.cannonsetmessages: + msg194974
2012-02-20 00:55:11brett.cannonsetmessages: + msg153746
2011-09-01 12:07:48pitrousetmessages: + msg143319
2011-09-01 07:23:11ncoghlansetmessages: + msg143309
2011-09-01 06:37:44neologixsetnosy: + ncoghlan, pitrou
messages: + msg143307
2011-09-01 05:06:58brett.cannonsetstatus: closed -> open
files: + socket_compiler_warning.diff


nosy: + brett.cannon
messages: + msg143298
resolution: fixed -> (no value)
stage: resolved -> commit review
2011-08-28 16:27:38neologixsetstatus: open -> closed
resolution: fixed
messages: + msg143103

stage: resolved
2011-08-28 16:22:26python-devsetnosy: + python-dev
messages: + msg143102
2011-08-28 15:56:43neologixsetmessages: + msg143099
2011-08-25 21:09:19baikiesetmessages: + msg142992
2011-08-24 22:35:11neologixsetmessages: + msg142936
2011-08-24 22:13:44neologixsetmessages: + msg142935
2011-08-24 22:04:58pitrousetnosy: + neologix
2011-08-24 21:56:45baikiecreate