inspector: reject reserved HashMap keys as Debugger scriptId - #36843
Draft
robobun wants to merge 1 commit into
Draft
inspector: reject reserved HashMap keys as Debugger scriptId#36843robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Conversation
Debugger.getScriptSource/searchInContent/setBreakpoint/getBreakpointLocations
parse the protocol scriptId to an unsigned SourceID and look it up directly
in m_scripts, an UncheckedKeyHashMap<unsigned, Script>. WTF integer hash
traits reserve 0 (empty) and UINT32_MAX (deleted) as sentinel keys, so a
client-supplied scriptId of "0" (or "-1"/"not-a-number"/overflow, which
value_or(0) collapses to 0) violates the table contract:
* asserts builds: ASSERTION FAILED: isValidKey(*entry), SIGABRT
* release: the lookup matches an empty bucket and returns a phantom
default-constructed Script; getScriptSource replies scriptSource:""
for a nonexistent script, and setBreakpoint dereferences the null
sourceProvider and SIGSEGVs.
Fixed in oven-sh/WebKit#386 by guarding each protocol-supplied
m_scripts.find() with isValidKey(), same as InspectorDOMAgent::nodeForId.
This commit carries the test coverage and the WEBKIT_VERSION bump to pick
up that fix.
Collaborator
Author
|
Status: waiting on the WebKit preview build. The fix lives in the WebKit prebuilt (oven-sh/WebKit#386), bumped here via The src-stash gate cannot observe fail-before for this change: the fix is not under |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on oven-sh/WebKit#386.
WEBKIT_VERSIONcurrently points at that PR's preview build and must be updated to the merged sha once it lands.Problem
InspectorDebuggerAgent::m_scriptsis anUncheckedKeyHashMap<JSC::SourceID, Script>. WTF's default integer hash traits reserve0as the empty-bucket key andUINT32_MAXas the deleted-bucket key. SeveralDebugger.*protocol handlers parse the client-suppliedscriptIdstring withparseIntegerAllowingTrailingJunk<uint32_t>(...).value_or(0)and pass the result directly tom_scripts.find().When an inspector client sends
scriptId: "0"(or"-1","not-a-number", or an overflow string, all of whichvalue_or(0)collapses to0):ASSERT_ENABLEDbuilds hitASSERTION FAILED: isValidKey(*entry)atwtf/HashTable.h:692and SIGABRT.Script:Debugger.getScriptSourcereplies{"scriptSource":""}for a script that does not exist, andDebugger.setBreakpointthen dereferences the phantom's nullsourceProviderand SIGSEGVs the debuggee.Repro
Debugger.searchInContent,Debugger.continueToLocationandDebugger.getBreakpointLocationstake the same path.Fix
oven-sh/WebKit#386 guards each protocol-supplied
m_scripts.find()withm_scripts.isValidKey(sourceID)so the reserved keys take the same "Missing script ..." error path as any other unknown id (same pattern asInspectorDOMAgent::nodeForId). This PR carries theWEBKIT_VERSIONbump and the regression tests.Verification
With the current prebuilt WebKit (fail-before,
bun bd):With oven-sh/WebKit#386 applied (pass-after,
bun run build:local):The fix lives in the WebKit prebuilt (bumped via
scripts/build/deps/webkit.ts), so the src-stash gate cannot observe fail-before; the proof above is the manual equivalent.Requires
--inspect+ the inspector token; loopback-bound by default.