Skip to content

script_bindings: Check for null pointer before dereferencing proxy handler custom data#36869

Merged
jdm merged 2 commits intomainfrom
jdm-patch-55
May 6, 2025
Merged

script_bindings: Check for null pointer before dereferencing proxy handler custom data#36869
jdm merged 2 commits intomainfrom
jdm-patch-55

Conversation

@jdm
Copy link
Copy Markdown
Member

@jdm jdm commented May 6, 2025

While the vast majority of DOM proxy objects created have a non-null pointer in the handler's extra data field, there is one place we create a proxy object that has a null pointer:

SyncWrapper(CreateProxyHandler(&traps, ptr::null()))
. Before #36818, dereferencing this null pointer was undefined behaviour that was silently being ignored; now that Rust 1.86 adds debug pointer validity checks, we get a panic when trying to dereference it.

Testing: Tested about:memory with rustc 1.86.

@jdm jdm mentioned this pull request May 6, 2025
Signed-off-by: Josh Matthews <[email protected]>
@jdm jdm enabled auto-merge May 6, 2025 05:57
@jdm jdm added this pull request to the merge queue May 6, 2025
Merged via the queue into main with commit 3b806ca May 6, 2025
21 checks passed
@jdm jdm deleted the jdm-patch-55 branch May 6, 2025 06:40
Narfinger pushed a commit to Narfinger/servo that referenced this pull request May 6, 2025
…ndler custom data (servo#36869)

While the vast majority of DOM proxy objects created have a non-null
pointer in the handler's extra data field, there is one place we create
a proxy object that has a null pointer:
https://github.com/servo/servo/blob/8b05b7449d3ceaeb6ae834246012ac04a4f048c6/components/script/window_named_properties.rs#L76
. Before servo#36818, dereferencing this null pointer was undefined behaviour
that was silently being ignored; now that Rust 1.86 adds debug pointer
validity checks, we get a panic when trying to dereference it.

Testing: Tested about:memory with rustc 1.86.

---------

Signed-off-by: Josh Matthews <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants