| msg121446 - (view) |
Author: Evgeny Kapun (abacabadabacaba) |
Date: 2010-11-18 07:18 |
| This code crashes Python: import io, mmap io.BytesIO(b' ').readinto(memoryview(mmap.mmap(-1, 1, prot=mmap.PROT_READ))) |
|
|
| msg126461 - (view) |
Author: Ross Lagerwall (rosslagerwall)  |
Date: 2011-01-18 12:38 |
| From what I can see, this issue is in memoryview and allows memoryview to export a readonly buffer as writable (because memoryview.getbuffer() removes the writable flag from flags before calling the underlying buffer). This causes segfaults when using mmap. If a bytes object is used as the underlying buffer, it allows the bytes object to be changed. Given this code: import io b=b"XXXX" m=memoryview(b) i=io.BytesIO(b'ZZZZ') i.readinto(m) print(b) print(b == b"XXXX") The output is: b'ZZZZ' True I think this is due to interning. Anyway, attached is a patch which hopefully fixes the issue + a test. |
|
|
| msg126468 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-18 15:59 |
| The patch produces a failure in test_getargs2, but that test is wrong and should be fixed: ====================================================================== ERROR: test_w_star (test.test_getargs2.Bytes_TestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/antoine/py3k/__svn__/Lib/test/test_getargs2.py", line 385, in test_w_star self.assertEqual(getargs_w_star(memoryview(b'memoryview')), b'[emoryvie]') TypeError: must be read-write buffer, not memoryview I'm surprised no other test failures arise. I did add that line (which I commented with "XXX for whatever reason...") for a reason, but I don't remember which one. It seemed necessary at the time, I'm glad it isn't anymore. So, about the patch itself: you should simply use assertRaises. There's no reason for readinto() not to fail with a TypeError (silent failure is wrong). Thank you. |
|
|
| msg126469 - (view) |
Author: Ross Lagerwall (rosslagerwall) |
Date: 2011-01-18 16:51 |
| Attached is an updated patch with a simpler test. |
|
|
| msg126470 - (view) |
Author: Ross Lagerwall (rosslagerwall) |
Date: 2011-01-18 17:01 |
| And a simple fix for the test_getargs2 test - it wraps the memoryview around a bytearray. |
|
|
| msg126472 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2011-01-18 18:19 |
| So, does "critical" mean "should be release blocker"? |
|
|
| msg126474 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-18 18:25 |
| > So, does "critical" mean "should be release blocker"? It's up to you to decide. It's not a new bug AFAICT. |
|
|
| msg126475 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2011-01-18 18:43 |
| The patch looks trivial enough. You're the memoryview guru, so if you have no doubts about it, I would say it can go in. |
|
|
| msg126476 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-18 19:07 |
| Patch with modified tests committed in r88097 (3.2), r88098 (3.1) and r88099 (2.7). Thank you! |
|
|