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

Issue 755144 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Indirect-leak in mojo::ScopedInterfaceEndpointHandle::ScopedInterfaceEndpointHandle

Project Member Reported by ClusterFuzz, Aug 14 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5104407799922688

Fuzzer: libFuzzer_mojo_parse_message_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Indirect-leak
Crash Address: 
Crash State:
  mojo::ScopedInterfaceEndpointHandle::ScopedInterfaceEndpointHandle
  mojo::AssociatedGroupController::CreateScopedInterfaceEndpointHandle
  mojo::internal::MultiplexRouter::CreateLocalEndpointHandle
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=492049:492114

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5104407799922688

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 
Cc: etienneb@chromium.org
Cc: msrchandra@chromium.org sa...@chromium.org
Components: Internals>Mojo
Labels: Test-Predator-Wrong-CLs M-62
Owner: yzshen@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL could not provide any possible suspects.
Using Code Search for the file, "scoped_interface_endpoint_handle.cc" assigning to the concern owner from GIT Blame.

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/2859a2ac06ab5d9df6706cc45525dc4a2085051c

@yzshen -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.

Comment 3 by yzshen@chromium.org, Aug 16 2017

Cc: och...@chromium.org tjbecker@google.com
I am not able to reproduce the issue locally by following the instructions.
And the report doesn't contain enough information for me to figure it out.

Tim and Oliver: Any suggestions how I could repro the issue? Thanks!

Comment 4 by och...@chromium.org, Aug 16 2017

Can you please try using our reproduce tool?

i.e. install https://github.com/google/clusterfuzz-tools#requirements

and then run:

`prodaccess && /google/data/ro/teams/clusterfuzz-tools/releases/clusterfuzz reproduce 5104407799922688`

Comment 5 by och...@chromium.org, Aug 16 2017

(i can reproduce this locally using the steps in #4)

Comment 6 by yzshen@chromium.org, Aug 16 2017

Thanks! I successfully reproed with the steps in #4. And looking at the output information of that, I found that to successfully repro using my own build I need to specify some env variables (didn't try to find out which ones are strictly necessary):

LSAN_OPTIONS="suppressions=/usr/local/google/home/yzshen/.pex/code/7fbf60af81569bd9faacbe00b3307b1dc714be6c/clusterfuzz/resources/suppressions/lsan_suppressions.txt:symbolize=1:external_symbolizer_path=/usr/local/google/home/yzshen/.pex/code/7fbf60af81569bd9faacbe00b3307b1dc714be6c/clusterfuzz/resources/llvm-symbolizer" ASAN_SYMBOLIZER_PATH="/usr/local/google/home/yzshen/.pex/code/7fbf60af81569bd9faacbe00b3307b1dc714be6c/clusterfuzz/resources/llvm-symbolizer" DISPLAY=":0.0" ASAN_OPTIONS="redzone=16:strict_string_check=1:strict_memcmp=1:allow_user_segv_handler=0:max_uar_stack_size_log=16:detect_stack_use_after_return=1:alloc_dealloc_mismatch=0:print_scariness=1:allocator_may_return_null=1:quarantine_size_mb=10:detect_odr_violation=0:handle_sigill=1:allocator_release_to_os_interval_ms=500:coverage=0:use_sigaltstack=1:fast_unwind_on_fatal=1:detect_leaks=1:handle_abort=1:check_malloc_usable_size=0:detect_container_overflow=1:symbolize=1:handle_segv=1" out/libfuzzer/mojo_parse_message_fuzzer ~/Downloads/clusterfuzz-testcase-5104407799922688 


Will continue looking...
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 19 2017

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

commit 91044a7378737cd78dcd5207d779a4a014179fd7
Author: Yuzhu Shen <yzshen@chromium.org>
Date: Sat Aug 19 03:41:04 2017

Mojo C++ bindings: avoid holding ScopedInterfaceEndpointHandles in MultiplexRouter.

Previously, we create ScopedInterfaceEndpointHandles for an incoming message
containing interface IDs as soon as it arrives. If the message is not dispatched
immediately it is stored in an internal queue of MultiplexRouter. That creates
circular references (because ScopedInterfaceEndpointHandle may hold a ref to
MultiplexRouter). Usually the queued messages are eventually dispatched which
breaks the circular references. But in some rare cases, the code fails to do
that and therefore leaks MultiplexRouter. For example,
- MultiplexRouter receives a PeerAssociatedEndpointClosed control message for
  interface ID x; it registers an endpoint for x.
- MultiplexRouter receives a message msg1 targetting interface ID x, because
  there is a registered endpoint without a client, it queues the message. If
  msg1 contains a payload interface ID y, it creates a
  ScopedInterfaceEndpointHandle for y.
- If MultiplexRouter never receives a message containing interface ID x, it will
  keep msg1 forever and therefore never get destoryed.

The above sequence can only happen if the remote side is not well behaved. But
having circular references is fragile. There could be other cases leading to
leaks.

This CL:
- makes sure ScopedInterfaceEndpointHandles are only created when messages
are dispatched.
- removes some unnecessary work on MultiplexRouter destruction.

A similar change for IPC::ChannelAssociatedGroupController will be in a
follow-up CL.

Bug:  755144 
Change-Id: Id58fe45e4fe0390773962dc25d81896d30589cb8
Reviewed-on: https://chromium-review.googlesource.com/619866
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Yuzhu Shen <yzshen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495797}
[modify] https://crrev.com/91044a7378737cd78dcd5207d779a4a014179fd7/mojo/public/cpp/bindings/lib/message.cc
[modify] https://crrev.com/91044a7378737cd78dcd5207d779a4a014179fd7/mojo/public/cpp/bindings/lib/multiplex_router.cc
[modify] https://crrev.com/91044a7378737cd78dcd5207d779a4a014179fd7/mojo/public/cpp/bindings/lib/multiplex_router.h

Project Member

Comment 8 by ClusterFuzz, Aug 19 2017

ClusterFuzz has detected this issue as fixed in range 495778:495801.

Detailed report: https://clusterfuzz.com/testcase?key=5104407799922688

Fuzzer: libFuzzer_mojo_parse_message_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Indirect-leak
Crash Address: 
Crash State:
  mojo::ScopedInterfaceEndpointHandle::ScopedInterfaceEndpointHandle
  mojo::AssociatedGroupController::CreateScopedInterfaceEndpointHandle
  mojo::internal::MultiplexRouter::CreateLocalEndpointHandle
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=492049:492114
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=495778:495801

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5104407799922688

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 9 by ClusterFuzz, Aug 19 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 5104407799922688 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment