New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 848809 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
please use my google.com address
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Some Mojo bindings uses can cause infinite recursion

Project Member Reported by roc...@chromium.org, Jun 1 2018

Issue description

Mojo adds a flag to its trap events indicating whether the event came from a local API call or from an external stimulus (i.e. a real IPC event).

SimpleWatcher in turn uses this, if woken up on the right sequence, to decide whether to dispatch synchronously or schedule a dispatch task asynchronously: if the event came from OOP it's safe to dispatch synchronously without fear of re-entrancy.

The synchronous dispatch however can in turn trigger yet more traps to be tripped and thus more events to be fired. These events inherit the source of the original event, so if the original event came from a real IPC, all nested events will also appear to come from real IPC and can thus synchronously dispatch as well.

This is wrong. Nested trap events should appear to come from local API calls. Otherwise it's trivial for e.g. a bindings error handler to trigger infinite recursion by running on the same sequence as internal IPC and e.g. attempting (and failling) to reconnect a pipe each time.

Easy fix, mojo::edk::RequestContext should explicitly use LOCAL_API_CALL as its source when nested within ~RequestContext.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/79f4898b566b0e580f7ebdf368ec52af5d3b9fa2

commit 79f4898b566b0e580f7ebdf368ec52af5d3b9fa2
Author: Ken Rockot <rockot@chromium.org>
Date: Fri Jun 01 20:24:38 2018

Mojo: don't inherit Source for nested RequestContext

Mojo trap events distinguish between local API call triggers and
triggers from external events (e.g. actual IPC). This signal is
leveraged to allow synchronous event dispatch without fear of
re-entrancy into Mojo implementation.

Trap events are aggregated in a thread-local RequestContext during the
extent of any interesting event (API call or incoming IPC) and then
dispatched during stack unwind to avoid re-entrancy.

The executed event handlers during this time may trigger further events,
which are thus accumulated by a new RequestContext, ad infinitum. This
means that chains of event handlers can cause arbitrarily deep
~RequestContext recursion.

Normally this is OK because higher-level Mojo library code does not
abuse the core APIs in a way that could ever actually cause such
recursion, with the exception of one case: external events which happen
to trigger an event handler on the same task sequence that processes
Mojo IPC. In this case, mojo::SimpleWatcher synchronously dispatches out
to user code which may do arbitrary stuff like bind or close new message
pipes. In such cases it's fairly trivial to construct some code which
triggers a stack overflow under certain (e.g. shutdown) conditions where
bindings are doomed to fail immediately.

This CL forces nested RequestContexts to dispatch their events as if
they came from local API calls, regardless of whether the context was
nested within an external event RequestContext, thus significantly
reducing the potential footgun surface at higher layers. This has the
side effect of forcing SimpleWatcher to schedule async dispatch tasks for
all but the outermost external event which triggers it.

Bug:  848809 
Change-Id: If4e4491ad887da73f3c23c8e9639478847c7f0c2
Reviewed-on: https://chromium-review.googlesource.com/1082921
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563774}
[modify] https://crrev.com/79f4898b566b0e580f7ebdf368ec52af5d3b9fa2/mojo/edk/system/request_context.cc

Status: Fixed (was: Started)

Sign in to add a comment