Should we fix these errors?
Somebody did some research and found some bugs in CPython (IIUC). The published some questionable fragments. If there's a volunteer we could probably easily fix these. (I know we already have occasional Coverity scans and there are other tools too (anybody try lgtm yet?) But this seems honest research (also Python leaves Ruby in the dust :-): http://www.viva64.com/en/b/0414/ -- --Guido van Rossum (python.org/~guido)
On Sat, Jul 23, 2016 at 12:36 AM, Guido van Rossum <guido@python.org> wrote:
Somebody did some research and found some bugs in CPython (IIUC). The published some questionable fragments. If there's a volunteer we could probably easily fix these. (I know we already have occasional Coverity scans and there are other tools too (anybody try lgtm yet?) But this seems honest research (also Python leaves Ruby in the dust :-):
First and foremost: All of these purported bugs appear to have been found by compiling on Windows. Does Coverity test a Windows build? If not, can we get it to? These look like the exact types of errors that Coverity *would* discover. Fragment N1 is accurate in current Python. (Although the wording of the report leaves something to be desired. "The SOCKET type in Windows is unsigned, so comparing it against null is meaningless." - only "x < 0" (not null) is meaningless.) It's lines 1702 and 2026 in current Python. What's the best solution? Create a macro VALID_SOCKET with two different definitions, one using "x < 0" and the other using "x != INVALID_SOCKET"? Fragment N2 doesn't appear to be in CPython 3.6 though. I can't find a file called a_print.c, nor anything with ASN1_PRINTABLE_type in it. Third party code? 2.7 only? I've no idea. (It'd be so much more helpful if file paths had been given instead of just fragment codes. The error messages include file names without paths in them.) Fragment N3: Looks like a legit issue. http://bugs.python.org/issue27591 created with patch. Fragment N4, N5, N6a: Can't find bn_lib.c, dh_ameth.c, or cms_env.c in the cpython tree anywhere. Google suggests that they could be part of OpenSSL (which could be true of a_print.c from N2). Does Python bundle any OpenSSL source anywhere? Fragment N6b (there's a completely unrelated issue paired up in N6): I don't understand all of what's being said here. The error message quoted refers to _elementtree.c:917, which is an understandable false positive for the static checker; the problem can't happen, though, because line 913 checks for NULL and will construct a new empty list, and line 916 iterates up to the new list's length, so line 917 can never be reached if self->extra is NULL. But their analyzer can't know that. On the other hand, the paragraph and code snippet are referring to _PyState_AddModule in Modules/pystate.c, which is never called with def=NULL anywhere else in CPython; unless it's intended to be public, the check on line 292 could simply be removed. Conclusion: CPython may need some better static checking in Windows mode, but probably not desperately enough to buy their product (which is presumably the point of that blog). ChrisA
On 2016-07-22 17:31, Chris Angelico wrote:
On Sat, Jul 23, 2016 at 12:36 AM, Guido van Rossum <guido@python.org> wrote:
Somebody did some research and found some bugs in CPython (IIUC). The published some questionable fragments. If there's a volunteer we could probably easily fix these. (I know we already have occasional Coverity scans and there are other tools too (anybody try lgtm yet?) But this seems honest research (also Python leaves Ruby in the dust :-):
First and foremost: All of these purported bugs appear to have been found by compiling on Windows. Does Coverity test a Windows build? If not, can we get it to? These look like the exact types of errors that Coverity *would* discover.
No, it doesn't. The Coverity Scan builds only run on X86_64 Linux platforms. When I took over Coverity Scan for CPython many years ago it was not possible to support multiple platforms and target with the free edition. I never tried to upload builds from different platforms because I feared that it might play havoc with the scan history. Should I check with Coverity again? Some of these issues have been found by Coverity and I even have patches for them, e.g. N6 is CID#1299595. I have 13 patches that I haven't published and merged yet. None of the issues is critical, though. Since I forgot how to use hg I have been waiting for the github migration. Christian
Oh, the first one is a regression that I introduced in the implementation of the PEP 475 (retry syscall on EINTR). I don't think that it can be triggered in practice, because socket handles on Windows are small numbers, so unlikely to be seen as negative. I just fixed it: https://hg.python.org/cpython/rev/6c11f52ab9db Victor 2016-07-22 16:36 GMT+02:00 Guido van Rossum <guido@python.org>:
Somebody did some research and found some bugs in CPython (IIUC). The published some questionable fragments. If there's a volunteer we could probably easily fix these. (I know we already have occasional Coverity scans and there are other tools too (anybody try lgtm yet?) But this seems honest research (also Python leaves Ruby in the dust :-):
http://www.viva64.com/en/b/0414/
-- --Guido van Rossum (python.org/~guido) _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/victor.stinner%40gmail.co...
On Fri, Jul 22, 2016, at 11:35, Victor Stinner wrote:
Oh, the first one is a regression that I introduced in the implementation of the PEP 475 (retry syscall on EINTR). I don't think that it can be triggered in practice, because socket handles on Windows are small numbers, so unlikely to be seen as negative.
The problem as I understand it isn't that handles will be seen as negative, the problem is that the error return will be seen as *non*-negative.
I just fixed it: https://hg.python.org/cpython/rev/6c11f52ab9db
Does INVALID_SOCKET exist on non-windows systems? (It's probably safe to compare against -1, the relevant functions are defined in POSIX as returning -1 rather than an unspecified negative value)
2016-07-22 18:21 GMT+02:00 Random832 <random832@fastmail.com>:
I just fixed it: https://hg.python.org/cpython/rev/6c11f52ab9db
Does INVALID_SOCKET exist on non-windows systems?
Yes, it was already used in almost all places. When I read again the code, in fact I found other places with "fd < 0" or "fd = -1". I fixed more code in a second change: https://hg.python.org/cpython/rev/025281485318 Victor
On 2016-07-22 16:36, Guido van Rossum wrote:
Somebody did some research and found some bugs in CPython (IIUC). The published some questionable fragments. If there's a volunteer we could probably easily fix these. (I know we already have occasional Coverity scans and there are other tools too (anybody try lgtm yet?) But this seems honest research (also Python leaves Ruby in the dust :-):
I had a closer look at the report. About half of the bugs, maybe more are not in the C code of CPython but in OpenSSL code. I really mean OpenSSL code, not _ssl.c and _hashopenssl.c. It's safe to assume that they forgot to exclude external dependencies. The issues in ASN1_PRINTABLE_type() [N2], BN_mask_bits() [N4 bn_lib.c, digest.c, evp_enc.c], dh_cms_set_peerkey() [N5, dh_ameth.c] and cms_env_set_version() [N6, cms_env.c] are all OpenSSL issues and should be reported to OpenSSL. Guido, did the company contact you or do you have Pavel Belikov's email address? Christian
FYI there is also a bug tracker report about this: https://bugs.python.org/issue27587 On 23 July 2016 at 13:22, Christian Heimes <christian@python.org> wrote:
On 2016-07-22 16:36, Guido van Rossum wrote:
Somebody did some research and found some bugs in CPython (IIUC). The published some questionable fragments. If there's a volunteer we could probably easily fix these. (I know we already have occasional Coverity scans and there are other tools too (anybody try lgtm yet?) But this seems honest research (also Python leaves Ruby in the dust :-):
I had a closer look at the report. About half of the bugs, maybe more are not in the C code of CPython but in OpenSSL code. I really mean OpenSSL code, not _ssl.c and _hashopenssl.c. It's safe to assume that they forgot to exclude external dependencies.
The issues in ASN1_PRINTABLE_type() [N2], BN_mask_bits() [N4 bn_lib.c, digest.c, evp_enc.c], dh_cms_set_peerkey() [N5, dh_ameth.c] and cms_env_set_version() [N6, cms_env.c] are all OpenSSL issues and should be reported to OpenSSL.
Guido, did the company contact you or do you have Pavel Belikov's email address?
Perhaps you can contact him via the email address at <https://bugs.python.org/user24304>.
participants (6)
-
Chris Angelico -
Christian Heimes -
Guido van Rossum -
Martin Panter -
Random832 -
Victor Stinner