Skip to content

fix: prevent use-after-free in permission request callbacks#50032

Merged
jkleinsc merged 1 commit into
mainfrom
wc-perm-uaf
Mar 3, 2026
Merged

fix: prevent use-after-free in permission request callbacks#50032
jkleinsc merged 1 commit into
mainfrom
wc-perm-uaf

Conversation

@codebytere

@codebytere codebytere commented Mar 2, 2026

Copy link
Copy Markdown
Member

Description of Change

EnterFullscreenModeForTab, RequestPointerLock, and RequestKeyboardLock bind callbacks with base::Unretained(this); fullscreen also captures a raw RenderFrameHost*. These callbacks may be invoked by the app's JS permission handler after the WebContents or RenderFrameHost is destroyed.

Use GetWeakPtr() in all three call sites, and capture a GlobalRenderFrameHostToken instead of the raw RenderFrameHost* for fullscreen so the pointer is resolved and null-checked only when the
callback fires. Cancel in-flight permission requests from ~WebContents() via ElectronPermissionManager::CancelPendingRequests() so stale callbacks are never handed back to JS.

Checklist

Release Notes

Notes: none

@codebytere
fix: prevent use-after-free in permission request callbacks
5225f4e 
EnterFullscreenModeForTab, RequestPointerLock, and RequestKeyboardLock
bind callbacks with base::Unretained(this); fullscreen also captures a
raw RenderFrameHost*. These callbacks may be invoked by the app's JS
permission handler after the WebContents or RenderFrameHost is destroyed.

Use GetWeakPtr() in all three call sites, and capture a
GlobalRenderFrameHostToken instead of the raw RenderFrameHost* for
fullscreen so the pointer is resolved and null-checked only when the
callback fires. Cancel in-flight permission requests from ~WebContents()
via a new ElectronPermissionManager::CancelPendingRequests()` so stale
callbacks are never handed back to JS.
@codebytere codebytere added semver/patch backwards-compatible bug fixes target/39-x-y PR should also be added to the "39-x-y" branch. target/40-x-y PR should also be added to the "40-x-y" branch. target/41-x-y PR should also be added to the "41-x-y" branch. labels Mar 2, 2026
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Mar 2, 2026
@jkleinsc jkleinsc merged commit 9f9a5b8 into main Mar 3, 2026
125 of 126 checks passed
@jkleinsc jkleinsc deleted the wc-perm-uaf branch March 3, 2026 00:01
@release-clerk

release-clerk Bot commented Mar 3, 2026

Copy link
Copy Markdown

No Release Notes

This was referenced Mar 3, 2026
Merged
Merged
@trop

trop Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "41-x-y", please check out #50034

@trop trop Bot added the in-flight/41-x-y label Mar 3, 2026
@trop trop Bot mentioned this pull request Mar 3, 2026
Merged
@trop trop Bot removed the target/41-x-y PR should also be added to the "41-x-y" branch. label Mar 3, 2026
@trop

trop Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "39-x-y", please check out #50035

@trop

trop Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "40-x-y", please check out #50036

@trop trop Bot added in-flight/39-x-y in-flight/40-x-y merged/40-x-y PR was merged to the "40-x-y" branch. merged/41-x-y PR was merged to the "41-x-y" branch. and removed target/39-x-y PR should also be added to the "39-x-y" branch. target/40-x-y PR should also be added to the "40-x-y" branch. in-flight/40-x-y in-flight/41-x-y labels Mar 3, 2026
@trop trop Bot added merged/39-x-y PR was merged to the "39-x-y" branch. and removed in-flight/39-x-y labels Mar 3, 2026
@MarshallOfSound

MarshallOfSound commented Mar 9, 2026

Copy link
Copy Markdown
Member

/trop run backport-to 38-x-y

@trop

trop Bot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

The backport process for this PR has been manually initiated - sending your PR to 38-x-y!

@trop

trop Bot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

I was unable to backport this PR to "38-x-y" cleanly;
you will need to perform this backport manually.

VerteDinde pushed a commit that referenced this pull request Mar 9, 2026
@codebytere @VerteDinde
fix: prevent use-after-free in permission request callbacks (#50032)
7a8e98b 
EnterFullscreenModeForTab, RequestPointerLock, and RequestKeyboardLock
bind callbacks with base::Unretained(this); fullscreen also captures a
raw RenderFrameHost*. These callbacks may be invoked by the app's JS
permission handler after the WebContents or RenderFrameHost is destroyed.

Use GetWeakPtr() in all three call sites, and capture a
GlobalRenderFrameHostToken instead of the raw RenderFrameHost* for
fullscreen so the pointer is resolved and null-checked only when the
callback fires. Cancel in-flight permission requests from ~WebContents()
via a new ElectronPermissionManager::CancelPendingRequests()` so stale
callbacks are never handed back to JS.
@VerteDinde VerteDinde mentioned this pull request Mar 9, 2026
Merged
@trop

trop Bot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

@VerteDinde has manually backported this PR to "38-x-y", please check out #50153

ckerr pushed a commit that referenced this pull request Mar 9, 2026
@VerteDinde @codebytere
fix: prevent use-after-free in permission request callbacks (#50153)
07a1e9c 
fix: prevent use-after-free in permission request callbacks (#50032)

EnterFullscreenModeForTab, RequestPointerLock, and RequestKeyboardLock
bind callbacks with base::Unretained(this); fullscreen also captures a
raw RenderFrameHost*. These callbacks may be invoked by the app's JS
permission handler after the WebContents or RenderFrameHost is destroyed.

Use GetWeakPtr() in all three call sites, and capture a
GlobalRenderFrameHostToken instead of the raw RenderFrameHost* for
fullscreen so the pointer is resolved and null-checked only when the
callback fires. Cancel in-flight permission requests from ~WebContents()
via a new ElectronPermissionManager::CancelPendingRequests()` so stale
callbacks are never handed back to JS.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
@trop trop Bot added merged/38-x-y PR was merged to the "38-x-y" branch. and removed in-flight/38-x-y labels Mar 9, 2026
@ckerr ckerr mentioned this pull request Apr 30, 2026
Merged
5 tasks
jkleinsc pushed a commit that referenced this pull request May 4, 2026
@ckerr
fix: try clearing InspectableWebContents delegate earlier (#51420)
aa72ddc 
Fix a crash that appears to be a DevTools callback to `DevToolsOpened()`
while the WebContents teardown is underway.

This PR re-applies 5bd2938 / #49406: the first thing the WebContents
destructor does is to clear the IWCV's delegate. That approach was
accidentaly circumvented a little by 9f9a5b8 / #50032 which added new
code to the beginning of the destructor before clearing the delgate.

Sample crash trace:

Received signal 11 SEGV_MAPERR 0000000001b8
 0 0x55b70ad996b2 base::debug::CollectStackTrace() [../../base/debug/stack_trace_posix.cc:1048:7]
 1 0x55b70ad81021 base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:280:20]
 2 0x55b70ad9906f base::debug::(anonymous namespace)::StackDumpSignalHandler() [../../base/debug/stack_trace_posix.cc:483:3]
 3 0x7fe851b19520 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x4251f)
 4 0x55b70ac8c60d base::internal::WeakReference::IsValid() [../../base/memory/weak_ptr.cc:74:0]
 5 0x55b7041101e8 electron::api::WebContents::DevToolsOpened() [../../base/memory/weak_ptr.h:238:32]
 6 0x55b7041f5141 electron::InspectableWebContents::LoadCompleted() [../../electron/shell/browser/ui/inspectable_web_contents.cc:632:27]
 7 0x55b704033be3 base::RepeatingCallback<>::Run() [../../base/functional/callback.h:343:12]
 8 0x55b712272d9a (anonymous namespace)::ParseAndHandle<>() [../../chrome/browser/devtools/devtools_embedder_message_dispatcher.cc:320:13]
 9 0x55b712272ec2 base::internal::Invoker<>::Run() [../../base/functional/bind_internal.h:673:12]
10 0x55b712272cf3 base::RepeatingCallback<>::Run() [../../base/functional/callback.h:343:12]
11 0x55b712272c36 DispatcherImpl::Dispatch() [../../chrome/browser/devtools/devtools_embedder_message_dispatcher.cc:389:48]
12 0x55b7041f89c6 electron::InspectableWebContents::HandleMessageFromDevToolsFrontend() [../../electron/shell/browser/ui/inspectable_web_contents.cc:962:33]
jkleinsc pushed a commit that referenced this pull request May 4, 2026
@trop @ckerr
fix: try clearing InspectableWebContents delegate earlier (#51474)
34dc833 
Fix a crash that appears to be a DevTools callback to `DevToolsOpened()`
while the WebContents teardown is underway.

This PR re-applies 5bd2938 / #49406: the first thing the WebContents
destructor does is to clear the IWCV's delegate. That approach was
accidentaly circumvented a little by 9f9a5b8 / #50032 which added new
code to the beginning of the destructor before clearing the delgate.

Sample crash trace:

Received signal 11 SEGV_MAPERR 0000000001b8
 0 0x55b70ad996b2 base::debug::CollectStackTrace() [../../base/debug/stack_trace_posix.cc:1048:7]
 1 0x55b70ad81021 base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:280:20]
 2 0x55b70ad9906f base::debug::(anonymous namespace)::StackDumpSignalHandler() [../../base/debug/stack_trace_posix.cc:483:3]
 3 0x7fe851b19520 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x4251f)
 4 0x55b70ac8c60d base::internal::WeakReference::IsValid() [../../base/memory/weak_ptr.cc:74:0]
 5 0x55b7041101e8 electron::api::WebContents::DevToolsOpened() [../../base/memory/weak_ptr.h:238:32]
 6 0x55b7041f5141 electron::InspectableWebContents::LoadCompleted() [../../electron/shell/browser/ui/inspectable_web_contents.cc:632:27]
 7 0x55b704033be3 base::RepeatingCallback<>::Run() [../../base/functional/callback.h:343:12]
 8 0x55b712272d9a (anonymous namespace)::ParseAndHandle<>() [../../chrome/browser/devtools/devtools_embedder_message_dispatcher.cc:320:13]
 9 0x55b712272ec2 base::internal::Invoker<>::Run() [../../base/functional/bind_internal.h:673:12]
10 0x55b712272cf3 base::RepeatingCallback<>::Run() [../../base/functional/callback.h:343:12]
11 0x55b712272c36 DispatcherImpl::Dispatch() [../../chrome/browser/devtools/devtools_embedder_message_dispatcher.cc:389:48]
12 0x55b7041f89c6 electron::InspectableWebContents::HandleMessageFromDevToolsFrontend() [../../electron/shell/browser/ui/inspectable_web_contents.cc:962:33]

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
jkleinsc pushed a commit that referenced this pull request May 5, 2026
@trop @ckerr
fix: try clearing InspectableWebContents delegate earlier (#51473)
152bae8 
Fix a crash that appears to be a DevTools callback to `DevToolsOpened()`
while the WebContents teardown is underway.

This PR re-applies 5bd2938 / #49406: the first thing the WebContents
destructor does is to clear the IWCV's delegate. That approach was
accidentaly circumvented a little by 9f9a5b8 / #50032 which added new
code to the beginning of the destructor before clearing the delgate.

Sample crash trace:

Received signal 11 SEGV_MAPERR 0000000001b8
 0 0x55b70ad996b2 base::debug::CollectStackTrace() [../../base/debug/stack_trace_posix.cc:1048:7]
 1 0x55b70ad81021 base::debug::StackTrace::StackTrace() [../../base/debug/stack_trace.cc:280:20]
 2 0x55b70ad9906f base::debug::(anonymous namespace)::StackDumpSignalHandler() [../../base/debug/stack_trace_posix.cc:483:3]
 3 0x7fe851b19520 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x4251f)
 4 0x55b70ac8c60d base::internal::WeakReference::IsValid() [../../base/memory/weak_ptr.cc:74:0]
 5 0x55b7041101e8 electron::api::WebContents::DevToolsOpened() [../../base/memory/weak_ptr.h:238:32]
 6 0x55b7041f5141 electron::InspectableWebContents::LoadCompleted() [../../electron/shell/browser/ui/inspectable_web_contents.cc:632:27]
 7 0x55b704033be3 base::RepeatingCallback<>::Run() [../../base/functional/callback.h:343:12]
 8 0x55b712272d9a (anonymous namespace)::ParseAndHandle<>() [../../chrome/browser/devtools/devtools_embedder_message_dispatcher.cc:320:13]
 9 0x55b712272ec2 base::internal::Invoker<>::Run() [../../base/functional/bind_internal.h:673:12]
10 0x55b712272cf3 base::RepeatingCallback<>::Run() [../../base/functional/callback.h:343:12]
11 0x55b712272c36 DispatcherImpl::Dispatch() [../../chrome/browser/devtools/devtools_embedder_message_dispatcher.cc:389:48]
12 0x55b7041f89c6 electron::InspectableWebContents::HandleMessageFromDevToolsFrontend() [../../electron/shell/browser/ui/inspectable_web_contents.cc:962:33]

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@VerteDinde VerteDinde VerteDinde approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees

No one assigned

Labels

merged/38-x-y PR was merged to the "38-x-y" branch. merged/39-x-y PR was merged to the "39-x-y" branch. merged/40-x-y PR was merged to the "40-x-y" branch. merged/41-x-y PR was merged to the "41-x-y" branch. new-pr 🌱 PR opened recently semver/patch backwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@codebytere @MarshallOfSound @jkleinsc @VerteDinde