classification
Title: Use after free in pystate.c
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, neologix, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-06-29 19:53 by christian.heimes, last changed 2013-07-01 21:43 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
tstate.patch christian.heimes, 2013-06-30 15:01 review
Messages (8)
msg192043 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-06-29 19:53
Coverity doesn't like the code in and I think it's right. Can somebody look into the matter and check Python 3.3, too?

http://hg.python.org/cpython/file/ac7bc6700ac3/Python/pystate.c#l376
http://hg.python.org/cpython/file/ac7bc6700ac3/Python/pystate.c#l394

10. freed_arg: "tstate_delete_common(PyThreadState *)" frees "tstate". 

395    tstate_delete_common(tstate);
   
11. Condition "autoInterpreterState", taking true branch
   
CID 1019639 (#1 of 1): Use after free (USE_AFTER_FREE)12. use_after_free: Using freed pointer "tstate".
396    if (autoInterpreterState && PyThread_get_key_value(autoTLSkey) == tstate)
397        PyThread_delete_key_value(autoTLSkey);
msg192064 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2013-06-30 08:12
Well, tstate is freed, but is not used afterwards, it's just comparing the pointer against the TLS.
msg192067 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-06-30 11:40
Seems it is not possible to write a test for this.
msg192073 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-06-30 15:01
I think it's unsafe. The address of a pointer should not be used once the pointer has been freed.

How about we reverse the order? At first we remove the key from TLS and then free the tstate.
msg192074 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-06-30 15:12
LGTM.
msg192081 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2013-06-30 17:34
> I think it's unsafe. The address of a pointer should not be used once the pointer has been freed.

Dereferencing a freed pointer is unsafe. A pointer is just an address,
there's nothing inherently unsafe with comparing a pointer with a
value. The only thing that could go wrong is if the same address was
reused in between which could end up screwing your code logic, but
since here the TLS can only be set by the current thread, I think it's
perfectly safe.

> How about we reverse the order? At first we remove the key from TLS and then free the tstate.

Looks good to me!
msg192151 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-01 21:36
I was talking about memory address reuse, too. After all the code doesn't access data at the pointer's address but rather uses the address as an identifier.

I agree that the code should not behave erroneous under current circumstances. The GIL ensures serialization and provides a memory barrier. But it's hard to tell what might happen in the future and in embedded applications.

It smells fishy. :)
msg192152 - (view) Author: Roundup Robot (python-dev) Date: 2013-07-01 21:43
New changeset ff30bf84b378 by Christian Heimes in branch '3.3':
Issue #18328: Reorder ops in PyThreadState_Delete*() functions. Now the
http://hg.python.org/cpython/rev/ff30bf84b378

New changeset ebe064e542ef by Christian Heimes in branch 'default':
Issue #18328: Reorder ops in PyThreadState_Delete*() functions. Now the
http://hg.python.org/cpython/rev/ebe064e542ef
History
Date User Action Args
2013-07-01 21:43:36christian.heimessetstatus: open -> closed
resolution: fixed
stage: needs patch -> resolved
2013-07-01 21:43:19python-devsetnosy: + python-dev
messages: + msg192152
2013-07-01 21:36:29christian.heimessetmessages: + msg192151
2013-06-30 17:34:14neologixsetmessages: + msg192081
2013-06-30 15:12:45serhiy.storchakasetmessages: + msg192074
2013-06-30 15:01:10christian.heimessetfiles: + tstate.patch
keywords: + patch
messages: + msg192073
2013-06-30 11:40:15serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg192067
stage: test needed -> needs patch
2013-06-30 08:12:57neologixsetnosy: + neologix
messages: + msg192064
2013-06-29 19:53:37christian.heimescreate