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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:21 | admin | set | github: 57046 |
2014-01-07 22:03:51 | brett.cannon | set | status: open -> closed resolution: fixed messages:
+ msg207608
|
2014-01-07 22:01:11 | python-dev | set | messages:
+ msg207606 |
2013-12-15 23:12:41 | baikie | set | messages:
+ msg206266 |
2013-12-13 18:03:23 | brett.cannon | set | messages:
+ msg206132 |
2013-12-13 16:55:53 | brett.cannon | set | files:
+ silence_compiler_warning.diff
messages:
+ msg206119 |
2013-12-05 17:22:46 | baikie | set | files:
+ socket-compare-cmsg_len_end.diff
messages:
+ msg205312 |
2013-12-05 16:07:03 | brett.cannon | set | assignee: brett.cannon |
2013-12-05 15:43:28 | pitrou | set | messages:
+ msg205308 |
2013-12-05 14:40:03 | brett.cannon | set | messages:
+ msg205302 |
2013-12-05 06:35:49 | neologix | set | messages:
+ msg205278 |
2013-12-05 00:34:28 | pitrou | set | messages:
+ msg205268 |
2013-12-04 08:09:07 | christian.heimes | set | nosy:
+ christian.heimes
messages:
+ msg205205 versions:
+ Python 3.4 |
2013-10-18 14:31:17 | brett.cannon | set | files:
+ pragma.diff
messages:
+ msg200262 |
2013-08-12 17:48:11 | brett.cannon | set | messages:
+ msg194974 |
2012-02-20 00:55:11 | brett.cannon | set | messages:
+ msg153746 |
2011-09-01 12:07:48 | pitrou | set | messages:
+ msg143319 |
2011-09-01 07:23:11 | ncoghlan | set | messages:
+ msg143309 |
2011-09-01 06:37:44 | neologix | set | nosy:
+ ncoghlan, pitrou messages:
+ msg143307
|
2011-09-01 05:06:58 | brett.cannon | set | status: 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:38 | neologix | set | status: open -> closed resolution: fixed messages:
+ msg143103
stage: resolved |
2011-08-28 16:22:26 | python-dev | set | nosy:
+ python-dev messages:
+ msg143102
|
2011-08-28 15:56:43 | neologix | set | messages:
+ msg143099 |
2011-08-25 21:09:19 | baikie | set | messages:
+ msg142992 |
2011-08-24 22:35:11 | neologix | set | messages:
+ msg142936 |
2011-08-24 22:13:44 | neologix | set | messages:
+ msg142935 |
2011-08-24 22:04:58 | pitrou | set | nosy:
+ neologix
|
2011-08-24 21:56:45 | baikie | create | |