cloudflare / cloudflare/workerd

Bug: Possible data race on InspectorService::isolates (main thread vs. inspector thread)

Open
#6,803 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
8.7k
Forks
739
Avg merge
2d 20h
Merged PRs (30d)
174

Description

# Summary

I was poking around the codebase with the new Mythos model and it flagged what looks like a thread-safety bug in the inspector so I thought it would be good to make an issue for it.

When the inspector is enabled, two threads end up touching the same `isolates` hashmap without a lock, with the main thread adding entries to it as workers register while the inspector thread reads it to answer `/json/list` (and erases dead entries). If a worker registers while the inspector thread is walking the map, the `insert()` can rehash it mid-iteration and lead to UB.

There is a mutex nearby, but it only guards the pointer to the inspector service, not the map itself.

# Where

`src/workerd/server/server.c++`:
* The map: `isolates` around line 1587

* Inspector thread reads/iterates/erases it in `request()` (lines 1450, 1514, 1482, 1551)

* Main thread inserts via `registerIsolate()` (line 1579)

* The mutex that only covers the pointer (line 1390)

# Notes

The inspector is off by default and dev-only and Worker JS can't reach this map. Just looks like it could crash workerd during local debugging.

Haven't reproduced it at runtime. Please let me know if there's something I missed that makes it safe.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.