This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Segmentation fault in bytearray tests
Type: crash Stage: patch review
Components: Interpreter Core Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: mark.dickinson, pitrou
Priority: critical Keywords: patch

Created on 2009-01-13 18:50 by pitrou, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
issue4935.patch pitrou, 2009-01-13 19:59
issue4935-2.patch pitrou, 2009-01-13 22:32
Messages (7)
msg79766 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-01-13 18:50
This happens on a 32-bit build on a 64-bit system, which happens to have
some interesting properties: for example, malloc() will happily allocate
memory larger than Py_SSIZE_T_MAX.

The crash is exactly triggered by the following snippet:

        if sys.maxsize < (1 << 32) and struct.calcsize('P') == 4:
            self.assertRaises(OverflowError,
                              self.marshal(b'\ta\n\tb').expandtabs,
sys.maxsize)
msg79772 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-01-13 19:59
Here is a patch. The idea is to use unsigned arithmetic instead of
signed, and to check for overflows by comparing with PY_SSIZE_T_MAX.

(the exact reason of the crash is unknown, it looks like either a
compiler bug or a mis-optimization as (i < 0) returns 0 while i is
-0x7FFFFFFF)
msg79778 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-01-13 21:55
What's the purpose of old_i?  It looks like it's never used for anything.

Other than than, the patch looks good to me.

I'd guess that the "if (i < 0)" was simply optimized away.  This isn't 
necessarily a compiler bug: if I understand correctly, a strict reading of 
the C standards says it's legitimate for a compiler to assume that code is 
written in such a way that signed-arithmetic overflow never happens, and 
gcc (for one) is known to take advantage of this.

Also, it would be nice to cleanup the whitespace in this function while 
you're fixing it;  at the moment it's showing me a mixture of tabs and 
spaces.
msg79779 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-01-13 22:08
Actually, what's the purpose of old_j?  It looks like that's not needed 
any more, either!
msg79781 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-01-13 22:32
You are right, old_i and old_j are unused, they were part of another
approach I tried and which failed.
Attaching new patch with these 2 variables removed, and the function
cleanly reindented (of course the patch is messier because of this).
msg79785 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-01-13 22:48
Looks fine;  I think this should be applied.

It seems as though your reindentation left trailing whitespace on
the blank lines in the function:  the svn commit hook might
complain about this...
msg79795 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-01-13 23:26
Committed in trunk, py3k, 2.6 and 3.0. Thanks!
History
Date User Action Args
2022-04-11 14:56:44adminsetgithub: 49185
2009-01-13 23:26:46pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg79795
2009-01-13 22:48:00mark.dickinsonsetresolution: accepted
messages: + msg79785
2009-01-13 22:32:07pitrousetfiles: + issue4935-2.patch
messages: + msg79781
2009-01-13 22:08:25mark.dickinsonsetmessages: + msg79779
2009-01-13 21:55:37mark.dickinsonsetnosy: + mark.dickinson
messages: + msg79778
2009-01-13 19:59:22pitrousetfiles: + issue4935.patch
keywords: + patch
messages: + msg79772
stage: patch review
2009-01-13 18:50:39pitroucreate