Skip to content

Commit

Permalink
Fix segfault in ThrottlingURLLoader
Browse files Browse the repository at this point in the history
The following sequence of events can cause a segfault:
  1. ThrottlingURLLoader::Start is called to kick off a request
  2. A throttle's WillStartRequest method redirects the request by
     changing ResourceRequest::url
  3. ThrottlingURLLoader::StartNow is called, possibly after deferral
  4. StartNow calls ThrottlingURLLoader::OnReceivedRedirect because the
     request URL was changed in step 2
  5. A throttle's WillRedirectRequest method defers the redirect
  6. When handling the defer, OnReceivedRedirect calls
     client_binding_.PauseIncomingMethodCallProcessing, which segaults
     because client_binding_ is unbound because the request never
     actually started

Bug: 933538
Change-Id: I0f7033d159d34601421da781f7902b6ae207c58a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1511620
Reviewed-by: John Abd-El-Malek <[email protected]>
Commit-Queue: Robbie McElrath <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#639178}(cherry picked from commit a86e86d90b35de30ed66b5f0337a082474a01913)
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1519287
Reviewed-by: Robbie McElrath <[email protected]>
Cr-Commit-Position: refs/branch-heads/3729@{#72}
Cr-Branched-From: d4a8972-refs/heads/master@{#638880}
  • Loading branch information
robbiemc committed Mar 13, 2019
1 parent af4ab8b commit 809cf1b
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions content/common/throttling_url_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,9 @@ void ThrottlingURLLoader::OnReceiveRedirect(
deferred_stage_ = DEFERRED_REDIRECT;
redirect_info_ =
std::make_unique<RedirectInfo>(redirect_info, response_head);
client_binding_.PauseIncomingMethodCallProcessing();
// |client_binding_| can be unbound if the redirect came from a throttle.
if (client_binding_.is_bound())
client_binding_.PauseIncomingMethodCallProcessing();
return;
}
}
Expand Down Expand Up @@ -678,7 +680,9 @@ void ThrottlingURLLoader::Resume() {
break;
}
case DEFERRED_REDIRECT: {
client_binding_.ResumeIncomingMethodCallProcessing();
// |client_binding_| can be unbound if the redirect came from a throttle.
if (client_binding_.is_bound())
client_binding_.ResumeIncomingMethodCallProcessing();
// TODO(dhausknecht) at this point we do not actually know if we commit to
// the redirect or if it will be cancelled. FollowRedirect would be a more
// suitable place to set this URL but there we do not have the data.
Expand Down

0 comments on commit 809cf1b

Please sign in to comment.