Skip to content

Commit

Permalink
Adds CHECKs in pursuit of message leaks
Browse files Browse the repository at this point in the history
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 <rockot@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538488}
  • Loading branch information
krockot authored and Commit Bot committed Feb 22, 2018
1 parent 2c1d42f commit 37ddd81
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 1 deletion.
9 changes: 8 additions & 1 deletion ipc/ipc_mojo_bootstrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 8 additions & 0 deletions mojo/edk/system/channel_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit 37ddd81

Please sign in to comment.