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

Issue 824144 link

Starred by 1 user
Status: Fixed
Owner:
█ xunji...@chromium.org
Last visit > 30 days ago
Closed: Mar 2018
Cc:
█ roc...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 821030



Sign in to add a comment

RenderFrameHostImpl::GetInterface operates on an invalid handle

Project Member Reported by xunji...@chromium.org, Mar 21 2018 
Steps to repro:
1. Apply https://chromium-review.googlesource.com/c/chromium/src/+/957918 which adds a DCHECK which triggers when a caller operates on an invalid handle.
2. Observe that many browser_tests will fail. E.g.  https://ci.chromium.org/buildbot/tryserver.chromium.mac/mac_chromium_rel_ng/674765

MediaRouterIntegrationBrowserTest.Fail_StartCancelledNoSinks
ChromeFindRequestManagerTest.FindInPDF
PaymentRequestCreditCardEditorTest.EditingExpiredCard
...

Call stack:

	base::debug::StackTrace::StackTrace [0x031148D0+32]
	base::debug::StackTrace::StackTrace [0x030DBBCD+13]
	logging::LogMessage::~LogMessage [0x03082A43+83]
	mojo::ScopedHandleBase<mojo::SharedBufferHandle>::operator-> [0x05B71480+78]
	content::RenderFrameHostImpl::GetInterface [0x01FA4957+71]
	service_manager::mojom::InterfaceProviderStubDispatch::Accept [0x03ADF386+310]
	service_manager::mojom::InterfaceProviderStub<mojo::RawPtrImplRefTraits<service_manager::mojom::InterfaceProvider> >::Accept [0x01CC5113+19]
	mojo::InterfaceEndpointClient::HandleValidatedMessage [0x03AA6935+541]
	mojo::FilterChain::Accept [0x03AAB593+131]
	mojo::InterfaceEndpointClient::HandleIncomingMessage [0x03AA76E2+106]
	mojo::internal::MultiplexRouter::ProcessIncomingMessage [0x03AA0CB0+698]
	mojo::internal::MultiplexRouter::Accept [0x03AA0837+295]
	mojo::FilterChain::Accept [0x03AAB593+131]
	mojo::Connector::ReadSingleMessage [0x03AA4880+364]
	mojo::Connector::ReadAllAvailableMessages [0x03AA4F69+87]
	mojo::Connector::OnHandleReadyInternal [0x03AA4E34+126]
	base::internal::Invoker<base::internal::BindState<void (__thiscall content::RedirectToFileResourceHandler::Writer::*)(int),base::internal::UnretainedWrapper<content::RedirectToFileResourceHandler::Writer> >,void __cdecl(int)>::RunOnce [0x0187827F+15]
	favicon::FaviconService::FaviconResultsCallbackRunner [0x045C4868+24]
	base::internal::Invoker<base::internal::BindState<void (__cdecl*)(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_strin [0x06486723+19]
	mojo::SimpleWatcher::OnHandleReady [0x03A16D40+224]
	base::internal::Invoker<base::internal::BindState<void (__thiscall mojo::SimpleWatcher::*)(int,unsigned int,mojo::HandleSignalsState const &),base::WeakPtr<mojo::SimpleWatcher>,int,unsigned int,mojo::HandleSignalsState>,void __cdecl(void)>::Run [0x03A16FBA+58]
	base::debug::TaskAnnotator::RunTask [0x0311144D+237]
	base::internal::IncomingTaskQueue::RunTask [0x03134E59+105]
	base::MessageLoop::RunTask [0x030B78F7+519]
	base::MessageLoop::DeferOrRunPendingTask [0x030B7C5D+157]
	base::MessageLoop::DoWork [0x030B7E8A+506]
	base::MessagePumpForUI::DoRunLoop [0x030F3A98+120]
	base::MessagePumpWin::Run [0x030F35FE+110]
	base::MessageLoop::Run [0x030B7299+169]

Comment 1 by xunji...@chromium.org, Mar 21 2018

Cc: roc...@chromium.org
+rockot@: Can you help to triage? I don't know this code and can't tell if there's anything wrong with RenderFrameHostImpl::GetInterface.

Comment 2 by roc...@chromium.org, Mar 21 2018 

RenderFrameHostImpl::GetInterface seems fine. Unfortunately that stack trace is not very useful. What I would need to see is what happens between GetInterface and the bad operator-> on a shared buffer handle (which is never called directly by GetInterface).

This means some interface binder registered on a registry (or delegated to via delegate_, TryBindFrameInterface, etc) is dereferencing a potentially null shared buffer handle.

A debug build should strip less code and should reveal a more detailed stack trace, I would guess?

Comment 3 by xunji...@chromium.org, Mar 21 2018

Owner: xunji...@chromium.org
Status: Assigned (was: Untriaged)
Thanks for the helpful background.

The issue is that interface_pipe->is_valid() is used instead of interface_pipe.is_valid() in RenderFrameHostImpl::GetInterface().

I will send you a CL.

Comment 4 by roc...@chromium.org, Mar 21 2018 

Ah, the "mojo::ScopedHandleBase<mojo::SharedBufferHandle>" in the stack confused me (I guess this is just bad template symbol deduplication?)

We have some signatures which take a ScopedMessagePipeHandle* so I didn't even noticed that the -> was wrong here. Good find!
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 21 2018 

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

commit 9463d54cee1fdbb52c285775b5d72cc8e58aad86
Author: Helen Li <xunjieli@chromium.org>
Date: Wed Mar 21 21:26:19 2018

Modify handle.is_valid() check in render_frame_host_impl.cc

handle->is_valid() will try to de-reference the handle. This CL changes
->is_valid() to .is_valid() so that the validity check is performed correctly.

Bug:  824144 
Change-Id: Ie6f7a5cfa2a7568cdbaceb30e0667d990d1326e3
Reviewed-on: https://chromium-review.googlesource.com/973914
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544851}
[modify] https://crrev.com/9463d54cee1fdbb52c285775b5d72cc8e58aad86/content/browser/frame_host/render_frame_host_impl.cc

Comment 6 by xunji...@chromium.org, Mar 22 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment