From 37ddd815d13ecfeffe1e365f8a167601493f81a3 Mon Sep 17 00:00:00 2001 From: Ken Rockot Date: Thu, 22 Feb 2018 18:18:46 +0000 Subject: [PATCH] Adds CHECKs in pursuit of message leaks It appears that we may in some cases be leaking message objects. Stacks from the wild indicate that these are often (if not always) legacy IPC messages, which means there's a good chance it's a leak either in the outgoing EDK channel backlog queue or in the IPC Channel's internal outgoing message queue. Bug: 813045 Change-Id: Id1663e36353c0f4325fbc53c829ded8d951a620c Reviewed-on: https://chromium-review.googlesource.com/926565 Commit-Queue: Ken Rockot Reviewed-by: Erik Chen Cr-Commit-Position: refs/heads/master@{#538488} --- ipc/ipc_mojo_bootstrap.cc | 9 ++++++++- mojo/edk/system/channel_win.cc | 8 ++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/ipc/ipc_mojo_bootstrap.cc b/ipc/ipc_mojo_bootstrap.cc index 6b0fd3b85af738..cb86bdaa0e6282 100644 --- a/ipc/ipc_mojo_bootstrap.cc +++ b/ipc/ipc_mojo_bootstrap.cc @@ -587,8 +587,15 @@ class ChannelAssociatedGroupController if (task_runner_->BelongsToCurrentThread()) { DCHECK(thread_checker_.CalledOnValidThread()); if (!connector_ || paused_) { - if (!shut_down_) + if (!shut_down_) { outgoing_messages_.emplace_back(std::move(*message)); + + // TODO(https://crbug.com/813045): Remove this. Typically this queue + // won't exceed something like 50 messages even on slow devices. If + // the massive leaks we see can be attributed to this queue, it would + // have to be quite a bit larger. + CHECK_LE(outgoing_messages_.size(), 100000u); + } return true; } return connector_->Accept(message); diff --git a/mojo/edk/system/channel_win.cc b/mojo/edk/system/channel_win.cc index b2c53968701787..980f0211c4c83b 100644 --- a/mojo/edk/system/channel_win.cc +++ b/mojo/edk/system/channel_win.cc @@ -106,6 +106,14 @@ class ChannelWin : public Channel, bool write_now = !delay_writes_ && outgoing_messages_.empty(); outgoing_messages_.emplace_back(std::move(message), 0); + // TODO(https://crbug.com/813045): Remove this. The queue should almost + // never be used, and then only for a handful of messages. For messages + // to accumulate, the sender would have to be sending faster than the + // receiver can read. This check is just to look for a message leak, + // though it may also reveal e.g. spammy malware sites exploiting some + // web APIs to DoS the browser. + CHECK_LE(outgoing_messages_.size(), 100000u); + if (write_now && !WriteNoLock(outgoing_messages_.front())) reject_writes_ = write_error = true; }