classification
Title: double free in io.TextIOWrapper
Type: crash Stage: resolved
Components: Extension Modules, IO Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: benjamin.peterson, python-dev, scufre, serhiy.storchaka, stutzbach
Priority: high Keywords: patch

Created on 2016-10-07 19:32 by scufre, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
textiowrapper_crash.py scufre, 2016-10-07 19:32 Proof of concept to demostrate the issue
textio.c.patch scufre, 2016-10-07 19:33 Proposed fix
textiowrapper_clear-3.5.patch serhiy.storchaka, 2016-10-30 09:54 review
textiowrapper_clear-2.7.patch serhiy.storchaka, 2016-10-30 09:54 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (3)
msg278263 - (view) Author: Sebastian Cufre (scufre) * Date: 2016-10-07 19:32
We have found that you can produce a crash when an instance of _io.TextIOWrapper is being deallocated while there's another thread invoking the garbage collector. I've attached a simple script that should reproduce the issue (textiowrapper_crash.py)

Looking at the code of the _io module, we found that on the dealloc method of the TextIOWrapper class, it first tries to invoke the close method, then releases its internal members, after that removes itself from the garbage collector tracking and finally frees deallocates the remaining stuff. What happens, is that while releasing it's internal members, it might end up calling code that releases the interpreter lock (because its doing an operating system call), letting other threads execute. If for example the thread that comes in, invokes the garbage collector, on debug will raise an assert on gcmodule.c on line 351, where I understand it is complaining that it is tracking an object with refcount 0. In a release build, I suppose this goes on and will end up releasing the object (given it has a refcount of 0), and when the interrupted dealloc thread continues will end up freeing again itself which at some point produces a crash.

Attached is a proposed fix for the issue in textio.c.patch, where the it the call to _PyObject_GC_UNTRACK is now done right after the call to the close method and before release its internal members.

As a reference we have been able to reproduce this with Python 2.7.12 on Windows (i386)
msg279717 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-10-30 09:54
Thank you for your report and patch Sebastian.

In Python 3 the solution can be simpler, just move the line "_PyObject_GC_UNTRACK(self);" above the line "textiowrapper_clear(self);". But calling PyObject_ClearWeakRefs() also should be moved up. Otherwise half-destroyed TextIOWrapper instance can be accessed via weak references. Following patches do this (and small refactoring).
msg279994 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-11-03 13:39
New changeset 91f024fc9b3a by Serhiy Storchaka in branch '2.7':
Issue #28387: Fixed possible crash in _io.TextIOWrapper deallocator when
https://hg.python.org/cpython/rev/91f024fc9b3a

New changeset 89f7386104e2 by Serhiy Storchaka in branch '3.5':
Issue #28387: Fixed possible crash in _io.TextIOWrapper deallocator when
https://hg.python.org/cpython/rev/89f7386104e2

New changeset c4319c0d0131 by Serhiy Storchaka in branch '3.6':
Issue #28387: Fixed possible crash in _io.TextIOWrapper deallocator when
https://hg.python.org/cpython/rev/c4319c0d0131

New changeset 36af3566b67a by Serhiy Storchaka in branch 'default':
Issue #28387: Fixed possible crash in _io.TextIOWrapper deallocator when
https://hg.python.org/cpython/rev/36af3566b67a
History
Date User Action Args
2017-03-31 16:36:23dstufftsetpull_requests: + pull_request972
2016-11-03 13:41:50serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-11-03 13:39:21python-devsetnosy: + python-dev
messages: + msg279994
2016-10-30 09:54:52serhiy.storchakasetfiles: + textiowrapper_clear-2.7.patch
2016-10-30 09:54:39serhiy.storchakasetfiles: + textiowrapper_clear-3.5.patch

messages: + msg279717
versions: + Python 3.5, Python 3.6, Python 3.7
2016-10-25 19:15:27serhiy.storchakasetassignee: serhiy.storchaka
2016-10-07 20:17:38serhiy.storchakasetpriority: normal -> high
nosy: + benjamin.peterson, stutzbach, serhiy.storchaka

components: + IO
stage: patch review
2016-10-07 19:33:52scufresetfiles: + textio.c.patch
keywords: + patch
2016-10-07 19:32:57scufrecreate