| msg193530 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2013-07-22 12:34 |
| Coverity claims that sock_accept() may leak a fd. I have been starring at the code for a while and I'm still not sure if Coverity is right. The macros make the code paths hard to follow. The attached patch is simple and should not be a performance issue. http://hg.python.org/cpython/file/01597384531f/Modules/socketmodule.c#l1965 6. open_fn: Returning handle opened by function "accept(int, __SOCKADDR_ARG, socklen_t * restrict)". 7. var_assign: Assigning: "newfd" = handle returned from "accept(s->sock_fd, __SOCKADDR_ARG({ .__sockaddr__ = &addrbuf.sa}), &addrlen)". CID 983312 (#1 of 1): Resource leak (RESOURCE_LEAK)14. overwrite_var: Overwriting handle "newfd" in "newfd = accept(s->sock_fd, __SOCKADDR_ARG({ .__sockaddr__ = &addrbuf.sa}), &addrlen)" leaks the handle. 1969 newfd = accept(s->sock_fd, SAS2SA(&addrbuf), &addrlen); |
|
|
| msg193531 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-07-22 13:06 |
| What if you add "errno = 0" at the beginning of the function? Perhaps Coverity is confused by the fact that we use CHECK_ERRNO() to infer whether accept() succeeded or not, rather than simply checking the return value. I don't like your patch, because it adds a code path that would really be a logic error and therefore should not pass silently. |
|
|
| msg193534 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-07-22 13:58 |
| Coverity may not understand the interaction with errno at all. Or it may simple point out that some operating systems are not standard conform and our assumption is not universally true. Here is a patch that raises an exception when newfd != INVALID_SOCKET. I would not mind if we close the issue with "false positive". The code seems to work just fine on all major platforms -- or we haven't seen the issue in the wild yet. |
|
|
| msg193535 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-07-22 14:00 |
| > Coverity may not understand the interaction with errno at all. Or it > may simple point out that some operating systems are not standard > conform and our assumption is not universally true. Have you tried to reset errno at the beginning of the function? |
|
|
| msg193537 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-07-22 14:18 |
| Am 22.07.2013 16:00, schrieb Antoine Pitrou: > Have you tried to reset errno at the beginning of the function? It doesn't affect Coverity's report: BEGIN_SELECT_LOOP(s) Py_BEGIN_ALLOW_THREADS + errno = 0; timeout = internal_select_ex(s, 0, interval); if (!timeout) { newfd = accept(s->sock_fd, SAS2SA(&addrbuf), &addrlen); |
|
|
| msg193549 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-07-22 16:56 |
| This patch does the trick. |
|
|
| msg193753 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-07-26 21:17 |
| http://bugs.python.org/file31016/closesock3.patch looks overkill. For example, BEGIN_SELECT_LOOP already sets errno to 0. Here is a simpler patch. |
|
|
| msg193754 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2013-07-26 21:23 |
| @pitrou: Does accept.patch look correct? I don't understand why we would call accept() more than once. |
|
|
| msg194579 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-08-06 21:11 |
| > I don't understand why we would call accept() more than once. Because of EINTR? |
|
|
| msg200722 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-10-21 08:50 |
| Shall I commit my patch or shall I close the coverity issue as intentionally? |
|
|
| msg203167 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-11-17 14:21 |
| The patch has been languishing for some time. Commit or close? |
|
|
| msg203171 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-11-17 14:26 |
| If none of use really understands the problem Coverity is trying to warn us about, I'd say "let languish or close". |
|
|
| msg203195 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2013-11-17 15:51 |
| OK, I'm rejecting it. |
|
|