Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843303002:
DOMStorage: Better defend against a tight loop maliciously using the API. (Closed)
|
Created:
3 years, 7 months ago by michaeln Modified:
3 years, 7 months ago Reviewers:
Marijn Kruisselbrink CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDOMStorage: Better defend against a tight loop maliciously using the API.
Terminate the renderer if an excessive number of calls are made,
This is indicative of script in an infinite loop or being malicious.
It's better to crash intentionally than by running the system OOM
and interfering with everything else running on the system.
BUG=706432
Review-Url: https://codereview.chromium.org/2843303002
Cr-Commit-Position: refs/heads/master@{#468502}
Committed: https://chromium.googlesource.com/chromium/src/+/8b561a879f76a6f776af31d13f90566c91f87600
Patch Set 1 #Patch Set 2 : comment #Patch Set 3 : 1000000 limit #Messages
Total messages: 22 (13 generated)
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== DOMStorage: Better defend against a tight loop maliciously using the API. Terminate the renderer if an excessive number of calls are made, This is indicative of script in an infinite loop or being malicious. It's better to crash intentionally than by running the system OOM and interfering with everything else running on the system. BUG=706432 ========== to ========== DOMStorage: Better defend against a tight loop maliciously using the API. Terminate the renderer if an excessive number of calls are made, This is indicative of script in an infinite loop or being malicious. It's better to crash intentionally than by running the system OOM and interfering with everything else running on the system. BUG=706432 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
michaeln@chromium.org changed reviewers: + mek@chromium.org
ptal, a very small and probably short lived change
How does this actually do something? PushPendingCallback is only called right before calls to SendThrottled, and SendThrottled will block if there are more than 1000 pending callbacks (until there are zero pending callbacks).
On 2017/04/28 17:02:39, Marijn Kruisselbrink wrote:
> How does this actually do something? PushPendingCallback is only called right
> before calls to SendThrottled, and SendThrottled will block if there are more
> than 1000 pending callbacks (until there are zero pending callbacks).
Glad you asked :) The way it works involves two threads, IPC thread and main
thread.
SendThrottled will block the main thread if there are 1000+ messages for which
the IPC thread has not received an ack, on receipt of each ack, a task is posted
to the main thread which will eventually pop and run a callback. But the IPC
thread makes progress independent of the main thread. If the main thread is not
making progress, the queue of callbacks will grow and grow. That growth is the
source of the memory exhaustion mentioned in the bug.
The existing mechanism in SendThrottle ensures a renderer can't swamp the
browser process with a flood of IPCs. The change in this CL to
PushPendingCallback ensures a renderer won't eat all of its memory (and the
system in general) with millions of of items in the pending_callbacks_ queue and
in the task queue.
while (true)
localStoage.setItem('lookma', 'itsinfinite!');
Code like that prevents the main thread from popping and running the callbacks.
SendThrottled and PushPendingCallback run on the main thread
MessageThrottlingFilter::OnMessageReceived() runs on the IPC thread
DomStorageDispatcher::OnMessageReceived() runs on the main thread
Also, as a matter of correctness (to maintain read stability), the mutation
events associated with DOMStorage must be delivered asyncly, after setItem() has
returned. So we can't relieve the memory pressure by running the callbacks prior
to setItem() returning.
makes sense, thanks for the explanation. lgtm Although as you point out, this fix isn't going to be very long lived (and there are many other ways a misbehaving/malicious website could have the same effect, so it's not obvious this throttling is even worth the effort).
The CQ bit was checked by michaeln@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/29 00:31:00, Marijn Kruisselbrink wrote: > makes sense, thanks for the explanation. lgtm > > Although as you point out, this fix isn't going to be very long lived (and there > are many other ways a misbehaving/malicious website could have the same effect, > so it's not obvious this throttling is even worth the effort). For me, for this particular problem, the combination of it being so easy induce and so easy to fix, leads me to going ahead and doing it.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by michaeln@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493680310615410,
"parent_rev": "8c6e6113a0f57941108e5267f699bd7dd740828b", "commit_rev":
"8b561a879f76a6f776af31d13f90566c91f87600"}
Message was sent while issue was closed.
Description was changed from ========== DOMStorage: Better defend against a tight loop maliciously using the API. Terminate the renderer if an excessive number of calls are made, This is indicative of script in an infinite loop or being malicious. It's better to crash intentionally than by running the system OOM and interfering with everything else running on the system. BUG=706432 ========== to ========== DOMStorage: Better defend against a tight loop maliciously using the API. Terminate the renderer if an excessive number of calls are made, This is indicative of script in an infinite loop or being malicious. It's better to crash intentionally than by running the system OOM and interfering with everything else running on the system. BUG=706432 Review-Url: https://codereview.chromium.org/2843303002 Cr-Commit-Position: refs/heads/master@{#468502} Committed: https://chromium.googlesource.com/chromium/src/+/8b561a879f76a6f776af31d13f90... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8b561a879f76a6f776af31d13f90... |
