Use after free in pystate.c

[tiran]

BPO	18328
Nosy	@tiran, @serhiy-storchaka
Files	tstate.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-07-01.21:43:36.851>
created_at = <Date 2013-06-29.19:53:37.930>
labels = ['interpreter-core', 'type-bug']
title = 'Use after free in pystate.c'
updated_at = <Date 2013-07-01.21:43:36.850>
user = 'https://github.com/tiran'

bugs.python.org fields:

activity = <Date 2013-07-01.21:43:36.850>
actor = 'christian.heimes'
assignee = 'none'
closed = True
closed_date = <Date 2013-07-01.21:43:36.851>
closer = 'christian.heimes'
components = ['Interpreter Core']
creation = <Date 2013-06-29.19:53:37.930>
creator = 'christian.heimes'
dependencies = []
files = ['30734']
hgrepos = []
issue_num = 18328
keywords = ['patch']
message_count = 8.0
messages = ['192043', '192064', '192067', '192073', '192074', '192081', '192151', '192152']
nosy_count = 4.0
nosy_names = ['christian.heimes', 'neologix', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18328'
versions = ['Python 3.3', 'Python 3.4']

[tiran]

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);

[neologix]

Well, tstate is freed, but is not used afterwards, it's just comparing the pointer against the TLS.

[serhiy-storchaka]

Seems it is not possible to write a test for this.

[tiran]

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.

[serhiy-storchaka]

LGTM.

[neologix]

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!

[tiran]

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. :)

[python-dev]

New changeset ff30bf84b378 by Christian Heimes in branch '3.3':
Issue bpo-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 bpo-18328: Reorder ops in PyThreadState_Delete*() functions. Now the
http://hg.python.org/cpython/rev/ebe064e542ef