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: memoryview can be used to write into readonly buffer
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: abacabadabacaba, georg.brandl, mark.dickinson, pitrou, rosslagerwall, vstinner
Priority: critical Keywords: patch

Created on 2010-11-18 07:18 by abacabadabacaba, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
i10451.patch rosslagerwall, 2011-01-18 12:38
i10451_v2.patch rosslagerwall, 2011-01-18 16:51 Simplify test
testfix.patch rosslagerwall, 2011-01-18 17:01
Messages (9)
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) (Python committer) 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) * (Python committer) 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) (Python committer) Date: 2011-01-18 16:51
Attached is an updated patch with a simpler test.
msg126470 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) 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) * (Python committer) Date: 2011-01-18 18:19
So, does "critical" mean "should be release blocker"?
msg126474 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2011-01-18 19:07
Patch with modified tests committed in r88097 (3.2), r88098 (3.1) and r88099 (2.7). Thank you!
History
Date User Action Args
2022-04-11 14:57:09adminsetgithub: 54660
2011-01-20 12:56:50vstinnersetnosy: + vstinner
2011-01-18 19:07:56pitrousetstatus: open -> closed
versions: + Python 2.7
nosy: georg.brandl, mark.dickinson, pitrou, abacabadabacaba, rosslagerwall
messages: + msg126476

resolution: fixed
stage: resolved
2011-01-18 18:43:19georg.brandlsetnosy: georg.brandl, mark.dickinson, pitrou, abacabadabacaba, rosslagerwall
messages: + msg126475
2011-01-18 18:25:40pitrousetnosy: georg.brandl, mark.dickinson, pitrou, abacabadabacaba, rosslagerwall
messages: + msg126474
2011-01-18 18:19:16georg.brandlsetnosy: georg.brandl, mark.dickinson, pitrou, abacabadabacaba, rosslagerwall
messages: + msg126472
2011-01-18 17:01:07rosslagerwallsetfiles: + testfix.patch
nosy: georg.brandl, mark.dickinson, pitrou, abacabadabacaba, rosslagerwall
messages: + msg126470
2011-01-18 16:51:46rosslagerwallsetfiles: + i10451_v2.patch
nosy: georg.brandl, mark.dickinson, pitrou, abacabadabacaba, rosslagerwall
messages: + msg126469
2011-01-18 16:02:25pitrousetpriority: normal -> critical
nosy: + georg.brandl
2011-01-18 15:59:38pitrousetnosy: mark.dickinson, pitrou, abacabadabacaba, rosslagerwall
messages: + msg126468
2011-01-18 13:29:05mark.dickinsonsetnosy: + mark.dickinson
2011-01-18 12:38:00rosslagerwallsetfiles: + i10451.patch

nosy: + rosslagerwall
messages: + msg126461

keywords: + patch
2010-11-18 13:38:51eric.araujosetnosy: + pitrou
2010-11-18 07:18:14abacabadabacabacreate