contextvars: hamt_alloc() must initialize h_root and h_count fields

[vstinner]

BPO	33803
Nosy	@vstinner, @ned-deily, @1st1
PRs	bpo-33803: Fix a crash in hamt.c #7504 [3.7] bpo-33803: Fix a crash in hamt.c (GH-7504) #7505

^{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 2018-06-08.03:44:16.205>
created_at = <Date 2018-06-07.23:44:54.469>
labels = ['interpreter-core', '3.7', '3.8', 'type-crash']
title = 'contextvars: hamt_alloc() must initialize h_root and h_count fields'
updated_at = <Date 2019-10-08.12:47:13.364>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2019-10-08.12:47:13.364>
actor = 'yselivanov'
assignee = 'none'
closed = True
closed_date = <Date 2018-06-08.03:44:16.205>
closer = 'yselivanov'
components = ['Interpreter Core']
creation = <Date 2018-06-07.23:44:54.469>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 33803
keywords = ['patch']
message_count = 12.0
messages = ['318990', '318991', '318992', '318994', '318997', '319001', '319013', '319026', '319032', '319033', '354172', '354199']
nosy_count = 3.0
nosy_names = ['vstinner', 'ned.deily', 'yselivanov']
pr_nums = ['7504', '7505']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue33803'
versions = ['Python 3.7', 'Python 3.8']

[vstinner]

test_asyncio started to crash in #7487

I debugged the crash with Yury: _PyHAMT_New() triggers indirectly a GC collection at "o->h_root = hamt_node_bitmap_new(0);". Problem: the object that is being created is already tracked by the GC, whereas its h_root field is not set. Then the GC does crash because the field is a random pointer.

hamt_alloc() must initialize h_root and h_count before tracking the object in the GC.

Thinking about the GC when writing an object constructor is hard :-(

Maybe we should add a debug option to trigger the GC more often to make such random bug more likely. contextvars has been implemented a few months ago, and we only spotted the bug a few days before Python 3.7.0 final release...

[vstinner]

One solution to trigger so crash more frequently is to reduce the threshold of the generation 0 of the garbage collector.

Here is a patch to do that when using -X dev: change the default threshold from 700 to 5 for the generation 0.

With this patch, "PYTHONDEVMODE=1 python -m test test_context" does also crash as expected.

diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index 958219b744..81218fb964 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -622,6 +622,10 @@ _Py_InitializeCore(const _PyCoreConfig *core_config)
         return _Py_INIT_ERR("runtime core already initialized");
     }
 
+    if (core_config->dev_mode) {
+        _PyRuntime.gc.generations[0].threshold = 5;
+    }
+
     /* Py_Finalize leaves _Py_Finalizing set in order to help daemon
      * threads behave a little more gracefully at interpreter shutdown.
      * We clobber it here so the new interpreter can start with a clean

[vstinner]

Don't forget to merge #7487 once this bug is fixed. I would like to see https://bugs.python.org/issue33792 in Python 3.7 *if possible* (the feature now seems "required" for the new asyncio.loop() function).

[1st1]

Thanks Victor for debugging this. I made a PR (which is now trivial) and double checked all other calls to GCTrack in context.c & hamt.c.

[1st1]

New changeset 378c53c by Yury Selivanov in branch 'master':
bpo-33803: Fix a crash in hamt.c (bpo-7504)
378c53c

[1st1]

New changeset a971a6f by Yury Selivanov (Miss Islington (bot)) in branch '3.7':
bpo-33803: Fix a crash in hamt.c (GH-7504) (GH-7505)
a971a6f

[1st1]

• if (core_config->dev_mode) {

•    \_PyRuntime.gc.generations[0].threshold = 5;
  

• }

I'd love to have a flag to turn this on, or maybe we should enable it for -X dev.

[vstinner]

I'd love to have a flag to turn this on, or maybe we should enable it for -X dev.

Well, there is already a public API to do it manually: gc.set_threshold(5).

I'm not sure about a threshold of 5 for -X dev: that's very very low and so kill performances. -X dev can be slower, not but 10x slower for example. I didn't measure performance.

Such bug is rare, so I'm not sure about putting gc.set_threshold(5) in -X dev.

[vstinner]

FYI the bug was also seen 8 hours ago on a different asyncio PR:
#423 (comment)