Issue 4935: Segmentation fault in bytearray tests
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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) * ![]() |
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:44 | admin | set | github: 49185 |
| 2009-01-13 23:26:46 | pitrou | set | status: open -> closed resolution: accepted -> fixed messages: + msg79795 |
| 2009-01-13 22:48:00 | mark.dickinson | set | resolution: accepted messages: + msg79785 |
| 2009-01-13 22:32:07 | pitrou | set | files:
+ issue4935-2.patch messages: + msg79781 |
| 2009-01-13 22:08:25 | mark.dickinson | set | messages: + msg79779 |
| 2009-01-13 21:55:37 | mark.dickinson | set | nosy:
+ mark.dickinson messages: + msg79778 |
| 2009-01-13 19:59:22 | pitrou | set | files:
+ issue4935.patch keywords: + patch messages: + msg79772 stage: patch review |
| 2009-01-13 18:50:39 | pitrou | create | |
