co_extra_freefuncs is stored thread locally and can lead to crashes

[DinoV]

BPO	30604
Nosy	@brettcannon, @vstinner, @ned-deily, @DinoV, @serhiy-storchaka, @1st1, @Mariatta, @jeethu
PRs	bpo-30604: Move co_extra_freefuncs to interpreter state to avoid crashes in threads #2015 bpo-30604: clean up co_extra support #2144 [3.6] bpo-30604: Fix __PyCodeExtraState_Get() prototype #2152 bpo-30604: Skip CoExtra tests if ctypes is missing #2356 [3.6] bpo-30604: Skip CoExtra tests if ctypes is missing (#2356) #2358 bpo-30704, bpo-30604: Fix memleak in code_dealloc() #2455 [3.6] bpo-30704, bpo-30604: Fix memleak in code_dealloc() (#2455) #2456

^{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 = 'https://github.com/DinoV'
closed_at = <Date 2017-06-23.13:26:09.274>
created_at = <Date 2017-06-08.21:37:03.405>
labels = ['interpreter-core', '3.7', 'type-crash']
title = 'co_extra_freefuncs is stored thread locally and can lead to crashes'
updated_at = <Date 2018-01-17.02:43:32.298>
user = 'https://github.com/DinoV'

bugs.python.org fields:

activity = <Date 2018-01-17.02:43:32.298>
actor = 'jeethu'
assignee = 'dino.viehland'
closed = True
closed_date = <Date 2017-06-23.13:26:09.274>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2017-06-08.21:37:03.405>
creator = 'dino.viehland'
dependencies = []
files = []
hgrepos = []
issue_num = 30604
keywords = ['patch']
message_count = 21.0
messages = ['295467', '295477', '295489', '295835', '295845', '295868', '296301', '296588', '296595', '296596', '296597', '296599', '296607', '296709', '296718', '296719', '297076', '297078', '297079', '297082', '297938']
nosy_count = 8.0
nosy_names = ['brett.cannon', 'vstinner', 'ned.deily', 'dino.viehland', 'serhiy.storchaka', 'yselivanov', 'Mariatta', 'jeethu']
pr_nums = ['2015', '2144', '2152', '2356', '2358', '2455', '2456']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue30604'
versions = ['Python 3.6', 'Python 3.7']

[DinoV]

The co_extra_freefuncs are stored in PyThreadState. When calling _PyEval_RequestCodeExtraIndex you are given a thread specific index. The code object can then lose it's last reference on a different thread, and the wrong free function can be called if users of the extra space have made calls to get their index in different orders.

This can also lead to crashes if the extra thread hasn't yet requested extra indexes either.

[ned-deily]

See review comments on PR 2015.

[serhiy-storchaka]

User code shouldn't allocate PyInterpreterState and PyThreadState structures, it only uses structures created by the interpreter. Changing the size of PyInterpreterState should be safe. The only possible breaking compatibility is if user code directly access co_extra_user_count, co_extra_freefuncs, async_gen_firstiter or async_gen_finalizer rather than using the API: _PyEval_RequestCodeExtraIndex(), _PyCode_GetExtra(), _PyCode_SetExtra(), _PyEval_GetAsyncGenFirstiter(), _PyEval_SetAsyncGenFirstiter(), _PyEval_GetAsyncGenFinalizer() and _PyEval_SetAsyncGenFinalizer().

Nick's idea about _preserve_36_ABI_1 and _preserve_36_ABI_2 should address concerns about direct access to async_gen_firstiter and async_gen_finalizer. Direct access to co_extra_user_count and co_extra_freefuncs obviously can't be preserved.

PyInterpreterState and PyThreadState are not in the stable ABI. They are opaque types when use limited API. May be they should be made opaque for all user code (in 3.7+).

[ned-deily]

New changeset 2997fec by Ned Deily (Dino Viehland) in branch '3.6':
[3.6] bpo-30604: Move co_extra_freefuncs to interpreter state to avoid crashes in threads (bpo-2015)
2997fec

[serhiy-storchaka]

Avoid using double underscores in C code. C compiler uses names with double underscores for its own needs, and this can lead to conflicts.

[vstinner]

New changeset 932946c by Victor Stinner in branch '3.6':
bpo-30604: Fix __PyCodeExtraState_Get() prototype (bpo-2152)
932946c

[Mariatta]

I'm setting the stage to 'backport needed', but it really is a 'porting needed' stage :)

The two PRs merged PRs here were made against 3.6.

[1st1]

New changeset f3cffd2 by Yury Selivanov (Dino Viehland) in branch 'master':
bpo-30604: clean up co_extra support (bpo-2144)
f3cffd2

[brettcannon]

Should this be closed since the all PRs got merged?

[Mariatta]

PR 2152 is not yet ported to master.

[1st1]

It doesn't need to be, it's only for 3.6