classification
Title: Possible integer overflow of PyLong_AsLong() results
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, asvetlov, eric.smith, haypo, jcea, mark.dickinson, python-dev, r.david.murray, rbcollins, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-09-20 19:01 by serhiy.storchaka, last changed 2015-09-06 20:31 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
long_aslong_overflow.patch serhiy.storchaka, 2012-09-21 16:53 review
long_aslong_overflow-3.3_2.patch serhiy.storchaka, 2012-09-23 19:15 review
long_aslong_overflow-3.2.patch serhiy.storchaka, 2012-10-03 14:03 review
long_aslong_overflow-2.7.patch serhiy.storchaka, 2012-10-03 14:03 review
long_aslong_overflow_tests.patch serhiy.storchaka, 2012-10-20 11:29 review
long_aslong_overflow-2.7_3.patch serhiy.storchaka, 2013-01-09 22:09 review
long_aslong_overflow-3.2_3.patch serhiy.storchaka, 2013-01-09 22:09 review
long_aslong_overflow-3.3_3.patch serhiy.storchaka, 2013-01-09 22:09 review
long_aslong_overflow-3.4_3.patch serhiy.storchaka, 2013-01-09 22:09 review
long_aslong_overflow_2-3.4.patch serhiy.storchaka, 2013-11-29 17:32 Uncommitted remnants without tests review
Messages (31)
msg170832 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-20 19:01
There are several places where the result of PyLong_AsLong() assigned to variable of type int. It can cause unknown issues on platforms with sizeof(int) != sizeof(long). All 140 cases of PyLong_AsLong() usage should be checked.
msg170847 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-20 20:49
Getting a C int out of a Python int is currently a bit awkward.  What do you think about adding a PyLong_AsInt counterpart to PyLong_AsLong?  (The alternative is to use PyLong_AsLong and repeat the same overflow detection code in many places.)
msg170850 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-20 21:02
In the Objects subdirectory (which is all I've looked at so far), I see issues in:

- fileobject.c (PyObject_AsFileDescriptor)

- structseq.c (PyLong_AsLong return value used as a Py_ssize_t;  probably safe, but it would be better to use PyLong_AsSsize_t).

- unicodeobject.c (one place where result assigned to something of type ssize_t, one where result assigned to something of type int).
msg170903 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-21 16:53
Here is a patch (for 3.3). I added private _PyLong_AsInt and where possible I 
have tried to use the appropriate function.
msg170904 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-21 16:59
I was assigned this to itself to show that I working on the issue. Now I free up place for someone with committing privileges.
msg171042 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-23 14:47
Looks good, in general.

_PyLong_AsInt should return -1 on overflow in all cases;  at the moment, it looks like it doesn't do that for values that overflow an int but not a long.
msg171046 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-23 15:38
I added some comments on Rietveld.

Apart from the bug in _PyLong_AsInt mentioned above, the patch mostly looks good.  But there are many changes, some of which will have user-visible effects, and I think it's somewhat risky to make all these changes in bugfix releases (which now includes Python 3.3).

Perhaps there could be a smaller patch aimed at the bugfix releases?  Ideally, each fix there should be backed by a regression test.
msg171078 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-23 19:15
Thank you for review and for found bugs. I again checked the patch, corrected 
the errors and dubious places. Also I corrected the error in 
Modules/grpmodule.c (in other places gid_t parsed as signed long) and the 
possible behaviour change in Modules/_io/_iomodule.c, reversed the changes in 
Modules/pyexpat.c. If some changes cause you have doubts, you can cherry-pick 
only the most obvious fixes.

> Ideally, each fix there should be backed by a regression test.

Unfortunately I have only platforms where sizeof(int) == sizeof(long) == 
sizeof(size_t).
msg171886 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-03 14:03
Added patches for 2.7 and 3.2.
msg173383 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-20 11:28
Here is a tests for most of fixed overflows. Some errors are difficult or impossible to reproduce.
msg179492 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-09 22:09
Here are minimized patches. All included changes have tests (i.e. overflow bugs 
are observable). I drop grp module changes even with tests, because there are 
other issues for this (issue2005, issue4591). New tests for fcntl added (they 
test PyObject_AsFileDescriptor).

If patches look good in general, I going first commit to 3.4, and if this will 
not break any buildbot,  then I'll commit the rest patches. After that I'll 
continue with not included changes (if I can write tests for them).
msg179991 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-14 23:15
New changeset 13e2e44db99d by Serhiy Storchaka in branch 'default':
Issue #15989: Fix several occurrences of integer overflow
http://hg.python.org/cpython/rev/13e2e44db99d
msg180005 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-15 09:10
Changeset 525407d89277: Fix test_socket broken in previous commit (changeset 13e2e44db99d).
msg180239 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-19 10:46
New changeset 974ace29ee2d by Serhiy Storchaka in branch '3.2':
Issue #15989: Fix several occurrences of integer overflow
http://hg.python.org/cpython/rev/974ace29ee2d

New changeset 8f10c9eae183 by Serhiy Storchaka in branch '3.3':
Issue #15989: Fix several occurrences of integer overflow
http://hg.python.org/cpython/rev/8f10c9eae183
msg180240 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-19 10:56
New changeset d544873d62e9 by Serhiy Storchaka in branch '2.7':
Issue #15989: Fix several occurrences of integer overflow
http://hg.python.org/cpython/rev/d544873d62e9
msg180245 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-01-19 13:02
Several 2.7 buildbots are failing.


> Unfortunately I have only platforms where sizeof(int) == sizeof(long) == 
sizeof(size_t).

You can use your cpython ssh key to login to all snakebite buildbot
machines. They are tagged with [SB].

http://mail.python.org/pipermail/python-dev/2012-September/121651.html
msg180250 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-19 19:10
New changeset a78ebf9aed06 by Serhiy Storchaka in branch '2.7':
Ensure that width and precision in string formatting test have type int, not long.
http://hg.python.org/cpython/rev/a78ebf9aed06
msg180252 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-19 19:14
Thank you for point, Stefan. And thanks Trent for his project.
msg180264 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-19 21:36
New changeset ee93a89b4e0f by Serhiy Storchaka in branch '2.7':
Issue #15989: Fix possible integer overflow in str formatting as in unicode formatting.
http://hg.python.org/cpython/rev/ee93a89b4e0f
msg180916 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-29 17:16
Sqlite module part extracted to issue17073.
msg204746 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-29 17:32
Here are remnants of initial patch which were not committed. There are no tests.
msg229621 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-18 00:12
Are the fixes in the final patch waiting for someone to write tests, or is the intent to commit them without tests after a final review because tests are too difficult to write?
msg229696 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-19 20:49
It would be good to write tests, but for some tests it is difficult (if possible). At least I did not see ways how to do this.
msg247373 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-25 19:03
@serhiy - I'm a little confused about the state of this patch. It seems like you need more review?
msg247374 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-25 19:05
Yes, it would be good if other's pair of eyes will look on the patch.
msg248339 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-09 21:49
It looks fine to me, for whatever thats worth. I think you should commit it.
msg250002 - (view) Author: Roundup Robot (python-dev) Date: 2015-09-06 18:26
New changeset d51a82f68a70 by Serhiy Storchaka in branch 'default':
Issue #15989: Fixed some scarcely probable integer overflows.
https://hg.python.org/cpython/rev/d51a82f68a70
msg250003 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-06 18:28
One of changes to Modules/_io/_iomodule.c is no longer actual since issue21679.

Change to Modules/selectmodule.c is no longer actual since issue23485 (f54bc2c52dfd).

The rest changes were committed to 3.6 only.
msg250009 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2015-09-06 19:02
Isn't Python-ast.c a generated file?
msg250017 - (view) Author: Roundup Robot (python-dev) Date: 2015-09-06 20:29
New changeset e53df7955192 by Serhiy Storchaka in branch 'default':
Make asdl_c.py to generate Python-ast.c changed in issue #15989.
https://hg.python.org/cpython/rev/e53df7955192
msg250018 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-06 20:31
> Isn't Python-ast.c a generated file?

Good catch Eric.
History
Date User Action Args
2015-09-06 20:31:06serhiy.storchakasetmessages: + msg250018
2015-09-06 20:29:50python-devsetmessages: + msg250017
2015-09-06 19:02:13eric.smithsetnosy: + eric.smith
messages: + msg250009
2015-09-06 18:28:50serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg250003

stage: commit review -> resolved
2015-09-06 18:26:24python-devsetmessages: + msg250002
2015-08-09 21:49:03rbcollinssetmessages: + msg248339
2015-07-25 19:05:30serhiy.storchakasetmessages: + msg247374
2015-07-25 19:03:15rbcollinssetnosy: + rbcollins
messages: + msg247373
2014-10-20 14:39:16Arfreversetnosy: + Arfrever
2014-10-19 20:49:53serhiy.storchakasetmessages: + msg229696
versions: + Python 3.5, - Python 3.3
2014-10-18 00:12:59r.david.murraysetnosy: + r.david.murray
messages: + msg229621
2014-05-22 22:01:31skrahsetnosy: - skrah
2013-11-29 17:33:05serhiy.storchakasetversions: - Python 3.2
2013-11-29 17:32:58serhiy.storchakasetfiles: + long_aslong_overflow_2-3.4.patch
nosy: + haypo
messages: + msg204746

2013-01-29 17:16:54serhiy.storchakasetmessages: + msg180916
2013-01-19 21:36:55python-devsetmessages: + msg180264
2013-01-19 19:14:46serhiy.storchakasetmessages: + msg180252
2013-01-19 19:10:35python-devsetmessages: + msg180250
2013-01-19 13:02:20skrahsetnosy: + skrah
messages: + msg180245
2013-01-19 10:56:49python-devsetmessages: + msg180240
2013-01-19 10:46:34python-devsetmessages: + msg180239
2013-01-15 09:10:33serhiy.storchakasetmessages: + msg180005
2013-01-14 23:17:09serhiy.storchakasetstage: patch review -> commit review
2013-01-14 23:15:51python-devsetnosy: + python-dev
messages: + msg179991
2013-01-09 22:09:10serhiy.storchakasetfiles: + long_aslong_overflow-2.7_3.patch, long_aslong_overflow-3.2_3.patch, long_aslong_overflow-3.3_3.patch, long_aslong_overflow-3.4_3.patch

messages: + msg179492
2012-12-29 22:07:20serhiy.storchakasetassignee: serhiy.storchaka
2012-12-20 13:01:52serhiy.storchakalinkissue16736 superseder
2012-11-29 14:37:00asvetlovsetnosy: + asvetlov
2012-10-24 09:04:56serhiy.storchakasetstage: patch review
2012-10-20 11:29:26serhiy.storchakasetfiles: + long_aslong_overflow_tests.patch
2012-10-20 11:28:43serhiy.storchakasetmessages: + msg173383
2012-10-16 08:38:57serhiy.storchakalinkissue15988 dependencies
2012-10-03 14:03:19serhiy.storchakasetfiles: + long_aslong_overflow-3.2.patch, long_aslong_overflow-2.7.patch

messages: + msg171886
2012-10-03 02:37:55jceasetnosy: + jcea
2012-09-23 19:15:40serhiy.storchakasetfiles: + long_aslong_overflow-3.3_2.patch

messages: + msg171078
2012-09-23 15:38:37mark.dickinsonsetmessages: + msg171046
versions: + Python 3.4
2012-09-23 14:47:05mark.dickinsonsetmessages: + msg171042
2012-09-21 16:59:26serhiy.storchakasetassignee: serhiy.storchaka -> (no value)
messages: + msg170904
2012-09-21 16:53:45serhiy.storchakasetfiles: + long_aslong_overflow.patch
keywords: + patch
messages: + msg170903
2012-09-20 21:02:02mark.dickinsonsetmessages: + msg170850
2012-09-20 20:49:19mark.dickinsonsetnosy: + mark.dickinson
messages: + msg170847
2012-09-20 19:01:45serhiy.storchakacreate