Issue metadata
Sign in to add a comment
|
Chrome: Crash Report - gfx::Image::ToImageSkia |
||||||||||||||||||||||
Issue descriptionThis crash : go/crash/d1e8b0f000000000, has been found by the latest SyzyASAN Canary(51.0.2663.1) Bad Access information: Error Typeheap-use-after-free Location0x18678067 Access Moderead Access Size4 User Size12 Magic Stack : Thread 0 CRASHED [EXCEPTION_BOUNDS_EXCEEDED @ 0x5376c7f7 ] MAGIC SIGNATURE THREAD 0x5376c7f7 (chrome.dll -image.cc:516 ) gfx::Image::ToImageSkia() 0x5376bdca (chrome.dll -image.cc:653 ) gfx::Image::AsImageSkia() 0x539ebd68 (chrome.dll -browser_tab_strip_controller.cc:501 ) BrowserTabStripController::SetTabRendererDataFromModel(content::WebContents *,int,TabRendererData *,BrowserTabStripController::TabStatus) 0x539ebce9 (chrome.dll -browser_tab_strip_controller.cc:518 ) BrowserTabStripController::SetTabDataAt(content::WebContents *,int) 0x539ec104 (chrome.dll -browser_tab_strip_controller.cc:473 ) BrowserTabStripController::TabChangedAt(content::WebContents *,int,TabStripModelObserver::TabChangeType) 0x539b7488 (chrome.dll -tab_strip_model.cc:501 ) TabStripModel::UpdateWebContentsStateAt(int,TabStripModelObserver::TabChangeType) 0x53967709 (chrome.dll -browser.cc:2281 ) Browser::ScheduleUIUpdate(content::WebContents *,unsigned int) 0x53965ca3 (chrome.dll -browser.cc:1538 ) Browser::NavigationStateChanged(content::WebContents *,content::InvalidateTypes) 0x53a57393 (chrome.dll -web_contents_impl.cc:1173 ) content::WebContentsImpl::NotifyNavigationStateChanged(content::InvalidateTypes) 0x53a569ab (chrome.dll -web_contents_impl.cc:3698 ) content::WebContentsImpl::LoadingStateChanged(bool,bool,content::LoadNotificationDetails *) 0x53a52fbc (chrome.dll -web_contents_impl.cc:4080 ) content::WebContentsImpl::DidStartLoading(content::FrameTreeNode *,bool) 0x53ac2364 (chrome.dll -frame_tree_node.cc:362 ) content::FrameTreeNode::DidStartLoading(bool,bool) 0x53a669b3 (chrome.dll -render_frame_host_impl.cc:1741 ) content::RenderFrameHostImpl::OnDidStartLoading(bool) 0x53a6ad9a (chrome.dll -ipc_message_templates.h:118 ) IPC::MessageT<RenderProcessHostMsg_SuddenTerminationChanged_Meta,std::tuple<bool>,void>::Dispatch<content::RenderProcessHostImpl,content::RenderProcessHostImpl,void,void ( content::RenderProcessHostImpl::*)(bool)>(IPC::Message const *,content::RenderProcessHostImpl *,content::RenderProcessHostImpl *,void *,void ( content::RenderProcessHostImpl::*)(bool)) 0x53a671ae (chrome.dll -render_frame_host_impl.cc:568 ) content::RenderFrameHostImpl::OnMessageReceived(IPC::Message const &) 0x53a7102b (chrome.dll -render_process_host_impl.cc:1774 ) content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const &) 0x53551f17 (chrome.dll -ipc_channel_proxy.cc:293 ) IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const &) 0x53a76658 (chrome.dll -bind_internal.h:354 ) base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<bool ( content::BrowserMessageFilter::Internal::*)(IPC::Message const &)>,void ,content::BrowserMessageFilter::Internal * const,IPC::Message const &>,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<bool ( content::BrowserMessageFilter::Internal::*)(IPC::Message const &)> >,void >::Run(base::internal::BindStateBase *) 0x527986f1 (chrome.dll -task_annotator.cc:51 ) base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask const &) 0x52722540 (chrome.dll -message_loop.cc:476 ) base::MessageLoop::RunTask(base::PendingTask const &) 0x52723872 (chrome.dll -message_loop.cc:597 ) base::MessageLoop::DoWork() 0x52797148 (chrome.dll -message_pump_win.cc:168 ) base::MessagePumpForUI::DoRunLoop() 0x52796c6d (chrome.dll -message_pump_win.cc:50 ) base::MessagePumpWin::Run(base::MessagePump::Delegate *) 0x5278234e (chrome.dll -run_loop.cc:35 ) base::RunLoop::Run() 0x532c6bff (chrome.dll -chrome_browser_main.cc:1809 ) ChromeBrowserMainParts::MainMessageLoopRun(int *) 0x53b0d8de (chrome.dll -browser_main_loop.cc:950 ) content::BrowserMainLoop::RunMainMessageLoopParts() 0x53b09c94 (chrome.dll -browser_main_runner.cc:152 ) content::BrowserMainRunnerImpl::Run() 0x53aad329 (chrome.dll -browser_main.cc:44 ) content::BrowserMain(content::MainFunctionParams const &) 0x533500fc (chrome.dll -content_main_runner.cc:395 ) content::RunNamedProcessTypeMain(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,content::MainFunctionParams const &,content::ContentMainDelegate *) 0x53350050 (chrome.dll -content_main_runner.cc:764 ) content::ContentMainRunnerImpl::Run() 0x5334d261 (chrome.dll -content_main.cc:19 ) content::ContentMain(content::ContentMainParams const &) 0x532549e2 (chrome.dll -chrome_main.cc:84 ) ChromeMain 0x00a8875a (chrome.exe -main_dll_loader_win.cc:183 ) MainDllLoader::Launch(HINSTANCE__ *) 0x00a87adf (chrome.exe -chrome_exe_main_win.cc:230 ) wWinMain 0x00abba09 (chrome.exe -crt0.c:251 ) __tmainCRTStartup 0x76b138f3 (kernel32.dll + 0x000138f3 ) BaseThreadInitThunk 0x77b05e12 (ntdll.dll + 0x00065e12 ) __RtlUserThreadStart 0x77b05ddd (ntdll.dll + 0x00065ddd ) _RtlUserThreadStart ASAN Free Stack Trace 0x569d951a (syzyasan_rtl.dll -block_heap_manager.cc:294 ) agent::asan::heap_managers::BlockHeapManager::Free(unsigned int,void *) 0x569dc80d (syzyasan_rtl.dll -rtl_impl.cc:123 ) asan_HeapFree 0x5435b4a4 (chrome.dll -free.c:51 ) free 0x5376b90b (chrome.dll + 0x0108b90b ) gfx::internal::ImageRepSkia::`scalar deleting destructor'(unsigned int) 0x53baf3a9 (chrome.dll -devtools_protocol_dispatcher.h:269 ) content::devtools::page::ScreencastVisibilityChangedParamsBuilder<1>::~ScreencastVisibilityChangedParamsBuilder<1>() 0x5376ba40 (chrome.dll -image.cc:755 ) gfx::Image::AddRepresentation(scoped_ptr<gfx::internal::ImageRep,std::default_delete<gfx::internal::ImageRep> >) 0x5376c7ed (chrome.dll -image.cc:516 ) gfx::Image::ToImageSkia() 0x5376bdcb (chrome.dll -image.cc:653 ) gfx::Image::AsImageSkia() 0x539ebd69 (chrome.dll -browser_tab_strip_controller.cc:501 ) BrowserTabStripController::SetTabRendererDataFromModel(content::WebContents *,int,TabRendererData *,BrowserTabStripController::TabStatus) 0x539ebcea (chrome.dll -browser_tab_strip_controller.cc:519 ) BrowserTabStripController::SetTabDataAt(content::WebContents *,int) 0x539ec105 (chrome.dll -browser_tab_strip_controller.cc:474 ) BrowserTabStripController::TabChangedAt(content::WebContents *,int,TabStripModelObserver::TabChangeType) 0x539b7489 (chrome.dll -tab_strip_model.cc:501 ) TabStripModel::UpdateWebContentsStateAt(int,TabStripModelObserver::TabChangeType) 0x5396770a (chrome.dll -browser.cc:2287 ) Browser::ScheduleUIUpdate(content::WebContents *,unsigned int) 0x53965ca4 (chrome.dll -browser.cc:1544 ) Browser::NavigationStateChanged(content::WebContents *,content::InvalidateTypes) 0x53a57394 (chrome.dll -web_contents_impl.cc:1173 ) content::WebContentsImpl::NotifyNavigationStateChanged(content::InvalidateTypes) 0x53a569ac (chrome.dll -web_contents_impl.cc:3700 ) content::WebContentsImpl::LoadingStateChanged(bool,bool,content::LoadNotificationDetails *) 0x53a52fbd (chrome.dll -web_contents_impl.cc:4087 ) content::WebContentsImpl::DidStartLoading(content::FrameTreeNode *,bool) 0x53ac2365 (chrome.dll -frame_tree_node.cc:366 ) content::FrameTreeNode::DidStartLoading(bool,bool) 0x53a669b4 (chrome.dll -render_frame_host_impl.cc:1741 ) content::RenderFrameHostImpl::OnDidStartLoading(bool) 0x53a6ad9b (chrome.dll -ipc_message_templates.h:119 ) IPC::MessageT<RenderProcessHostMsg_SuddenTerminationChanged_Meta,std::tuple<bool>,void>::Dispatch<content::RenderProcessHostImpl,content::RenderProcessHostImpl,void,void ( content::RenderProcessHostImpl::*)(bool)>(IPC::Message const *,content::RenderProcessHostImpl *,content::RenderProcessHostImpl *,void *,void ( content::RenderProcessHostImpl::*)(bool)) 0x53a671af (chrome.dll -render_frame_host_impl.cc:568 ) content::RenderFrameHostImpl::OnMessageReceived(IPC::Message const &) 0x53a7102c (chrome.dll -render_process_host_impl.cc:1774 ) content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const &) 0x53551f18 (chrome.dll -ipc_channel_proxy.cc:294 ) IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const &) 0x53a76659 (chrome.dll -bind_internal.h:354 ) base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<bool ( content::BrowserMessageFilter::Internal::*)(IPC::Message const &)>,void ,content::BrowserMessageFilter::Internal * const,IPC::Message const &>,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<bool ( content::BrowserMessageFilter::Internal::*)(IPC::Message const &)> >,void >::Run(base::internal::BindStateBase *) 0x527986f2 (chrome.dll -task_annotator.cc:51 ) base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask const &) 0x52722541 (chrome.dll -message_loop.cc:478 ) base::MessageLoop::RunTask(base::PendingTask const &) 0x52723873 (chrome.dll -message_loop.cc:598 ) base::MessageLoop::DoWork() 0x52797149 (chrome.dll -message_pump_win.cc:169 ) base::MessagePumpForUI::DoRunLoop() 0x52796c6e (chrome.dll -message_pump_win.cc:52 ) base::MessagePumpWin::Run(base::MessagePump::Delegate *) 0x5278234f (chrome.dll -run_loop.cc:36 ) base::RunLoop::Run() 0x532c6c00 (chrome.dll -chrome_browser_main.cc:1811 ) ChromeBrowserMainParts::MainMessageLoopRun(int *) 0x53b0d8df (chrome.dll -browser_main_loop.cc:952 ) content::BrowserMainLoop::RunMainMessageLoopParts() 0x53b09c95 (chrome.dll -browser_main_runner.cc:153 ) content::BrowserMainRunnerImpl::Run() 0x53aad32a (chrome.dll -browser_main.cc:44 ) content::BrowserMain(content::MainFunctionParams const &) 0x533500fd (chrome.dll -content_main_runner.cc:395 ) content::RunNamedProcessTypeMain(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,content::MainFunctionParams const &,content::ContentMainDelegate *) 0x53350051 (chrome.dll -content_main_runner.cc:764 ) content::ContentMainRunnerImpl::Run() 0x5334d262 (chrome.dll -content_main.cc:19 ) content::ContentMain(content::ContentMainParams const &) 0x532549e3 (chrome.dll -chrome_main.cc:87 ) ChromeMain 0x00a8875b (chrome.exe -main_dll_loader_win.cc:184 ) MainDllLoader::Launch(HINSTANCE__ *) 0x00a87ae0 (chrome.exe -chrome_exe_main_win.cc:231 ) wWinMain 0x00abba0a (chrome.exe -crt0.c:251 ) __tmainCRTStartup 0x76b138f4 (kernel32.dll + 0x000138f4 ) BaseThreadInitThunk 0x77b05e13 (ntdll.dll + 0x00065e13 ) __RtlUserThreadStart 0x77b05dde (ntdll.dll + 0x00065dde ) _RtlUserThreadStart ASAN Allocation Stack Trace 0x569d921e (syzyasan_rtl.dll -block_heap_manager.cc:190 ) agent::asan::heap_managers::BlockHeapManager::Allocate(unsigned int,unsigned int) 0x569dc763 (syzyasan_rtl.dll -rtl_impl.cc:102 ) asan_HeapAlloc 0x5435eb96 (chrome.dll -malloc.c:92 ) malloc 0x5435a3cd (chrome.dll -new.cpp:59 ) operator new(unsigned int) 0x5376c78b (chrome.dll -image.cc:488 ) gfx::Image::ToImageSkia() 0x5376bdcb (chrome.dll -image.cc:653 ) gfx::Image::AsImageSkia() 0x539ebd69 (chrome.dll -browser_tab_strip_controller.cc:501 ) BrowserTabStripController::SetTabRendererDataFromModel(content::WebContents *,int,TabRendererData *,BrowserTabStripController::TabStatus) 0x539ebcea (chrome.dll -browser_tab_strip_controller.cc:519 ) BrowserTabStripController::SetTabDataAt(content::WebContents *,int) 0x539ec105 (chrome.dll -browser_tab_strip_controller.cc:474 ) BrowserTabStripController::TabChangedAt(content::WebContents *,int,TabStripModelObserver::TabChangeType) 0x539b7489 (chrome.dll -tab_strip_model.cc:501 ) TabStripModel::UpdateWebContentsStateAt(int,TabStripModelObserver::TabChangeType) 0x5396770a (chrome.dll -browser.cc:2287 ) Browser::ScheduleUIUpdate(content::WebContents *,unsigned int) 0x53965ca4 (chrome.dll -browser.cc:1544 ) Browser::NavigationStateChanged(content::WebContents *,content::InvalidateTypes) 0x53a57394 (chrome.dll -web_contents_impl.cc:1173 ) content::WebContentsImpl::NotifyNavigationStateChanged(content::InvalidateTypes) 0x53a569ac (chrome.dll -web_contents_impl.cc:3700 ) content::WebContentsImpl::LoadingStateChanged(bool,bool,content::LoadNotificationDetails *) 0x53a52fbd (chrome.dll -web_contents_impl.cc:4087 ) content::WebContentsImpl::DidStartLoading(content::FrameTreeNode *,bool) 0x53ac2365 (chrome.dll -frame_tree_node.cc:366 ) content::FrameTreeNode::DidStartLoading(bool,bool) 0x53a669b4 (chrome.dll -render_frame_host_impl.cc:1741 ) content::RenderFrameHostImpl::OnDidStartLoading(bool) 0x53a6ad9b (chrome.dll -ipc_message_templates.h:119 ) IPC::MessageT<RenderProcessHostMsg_SuddenTerminationChanged_Meta,std::tuple<bool>,void>::Dispatch<content::RenderProcessHostImpl,content::RenderProcessHostImpl,void,void ( content::RenderProcessHostImpl::*)(bool)>(IPC::Message const *,content::RenderProcessHostImpl *,content::RenderProcessHostImpl *,void *,void ( content::RenderProcessHostImpl::*)(bool)) 0x53a671af (chrome.dll -render_frame_host_impl.cc:568 ) content::RenderFrameHostImpl::OnMessageReceived(IPC::Message const &) 0x53a7102c (chrome.dll -render_process_host_impl.cc:1774 ) content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const &) 0x53551f18 (chrome.dll -ipc_channel_proxy.cc:294 ) IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const &) 0x53a76659 (chrome.dll -bind_internal.h:354 ) base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<bool ( content::BrowserMessageFilter::Internal::*)(IPC::Message const &)>,void ,content::BrowserMessageFilter::Internal * const,IPC::Message const &>,base::internal::InvokeHelper<0,void,base::internal::RunnableAdapter<bool ( content::BrowserMessageFilter::Internal::*)(IPC::Message const &)> >,void >::Run(base::internal::BindStateBase *) 0x527986f2 (chrome.dll -task_annotator.cc:51 ) base::debug::TaskAnnotator::RunTask(char const *,base::PendingTask const &) 0x52722541 (chrome.dll -message_loop.cc:478 ) base::MessageLoop::RunTask(base::PendingTask const &) 0x52723873 (chrome.dll -message_loop.cc:598 ) base::MessageLoop::DoWork() 0x52797149 (chrome.dll -message_pump_win.cc:169 ) base::MessagePumpForUI::DoRunLoop() 0x52796c6e (chrome.dll -message_pump_win.cc:52 ) base::MessagePumpWin::Run(base::MessagePump::Delegate *) 0x5278234f (chrome.dll -run_loop.cc:36 ) base::RunLoop::Run() 0x532c6c00 (chrome.dll -chrome_browser_main.cc:1811 ) ChromeBrowserMainParts::MainMessageLoopRun(int *) 0x53b0d8df (chrome.dll -browser_main_loop.cc:952 ) content::BrowserMainLoop::RunMainMessageLoopParts() 0x53b09c95 (chrome.dll -browser_main_runner.cc:153 ) content::BrowserMainRunnerImpl::Run() 0x53aad32a (chrome.dll -browser_main.cc:44 ) content::BrowserMain(content::MainFunctionParams const &) 0x533500fd (chrome.dll -content_main_runner.cc:395 ) content::RunNamedProcessTypeMain(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,content::MainFunctionParams const &,content::ContentMainDelegate *) 0x53350051 (chrome.dll -content_main_runner.cc:764 ) content::ContentMainRunnerImpl::Run() 0x5334d262 (chrome.dll -content_main.cc:19 ) content::ContentMain(content::ContentMainParams const &) 0x532549e3 (chrome.dll -chrome_main.cc:87 ) ChromeMain 0x00a8875b (chrome.exe -main_dll_loader_win.cc:184 ) MainDllLoader::Launch(HINSTANCE__ *) 0x00a87ae0 (chrome.exe -chrome_exe_main_win.cc:231 ) wWinMain 0x00abba0a (chrome.exe -crt0.c:251 ) __tmainCRTStartup 0x76b138f4 (kernel32.dll + 0x000138f4 ) BaseThreadInitThunk 0x77b05e13 (ntdll.dll + 0x00065e13 ) __RtlUserThreadStart 0x77b05dde (ntdll.dll + 0x00065dde ) _RtlUserThreadStart This was only seen on ASAN builds which was introduced on 50.0.2630.1(2 crash instances) and 51.0.2663.1(5 crash instance). Crashes are observed on below builds : 50.0.2630.1 28.57% 2 51.0.2663.1 71.43% 5 Total: 100.00% 7 https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27browser%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27gfx%3A%3AImage%3A%3AToImageSkia%27%20OMIT%20RECORD%20IF%20SUM(ProductData.key%3D%27asan-error-type%27)%20%3D%200%20OR%20SUM(ProductData.value%3D%27corrupt-heap%27)%20%3E%200%20OR%20SUM(ProductData.value%3D%27corrupt-block%27)%20%3E%200%20OR%20SUM(ProductData.value%3D%27wild-access%27)%20%3E%200%20OR%20SUM(ProductData.value%3D%27heap-unknown-error%27)%20%3E%200%20OR%20SUM(ProductData.value%3D%27invalid-address%27)%20%3E%200&ignore_case=false Cc'ing : Chromium/src/chrome/browser/ui/views/tabs/OWNERS and chromium/src/ui/gfx/image/OWNERS
,
Mar 1 2016
That if block is executed only if the rep does not exist, so I'm a bit puzzled how this can happen. +mgiuca@ how made scoped_ptr change.
,
Mar 4 2016
I can see how this is theoretically possible. Not sure how to trigger it yet, but I think sky is right about the cause. It's dodgy to assume the pointer to rep is still alive after inserting it into the map (I added that assumption at https://codereview.chromium.org/1099473003). I will investigate and try to write a test case that crashes.
,
Mar 4 2016
OK I've analysed this a bunch. == Summary== Basically: There is a use-after-free possible in the event of a race condition (which I suspect is the cause). If ToImageSkia is called on the same Image from two threads, this can occur. I've looked at the two cases and I can't figure out how a race condition can happen, but there is a huge amount of complexity behind both of them. So, we can easily patch this to prevent this use-after-free. However, gfx::Image is not thread-safe by design (see Issue 468010 ), so this implies there is some code assuming thread safety that isn't there, and I can't figure that out. == Details == There are two separate indirect causes of this crash: 1. BrowserTabStripController on UI thread (e.g., crash/2c1490f000000000). 2. (web_app_win.cc)::GetImageCheckSum on FILE thread (e.g., crash/f7af02f000000000). The allocation point (in both cases) is image.cc:488 (creating a new ImageRepSkia from an ImageRepPNG). The free point in both cases is image.cc:516 (the scoped_rep destructor after the call to AddRepresentation), suggesting that AddRepresentation got the moved scoped_rep, and dropped it on the floor (i.e., it did not insert it into the map). Once you're in ToImageSkia, in a single thread, I don't see a way to trigger this case (like Oshima said). If GetRepresentation returns non-null, it won't call AddRepresentation, can't crash. So GetRepresentation must be returning null (no kImageRepSkia key in the map). Then we create a new ImageSkiaRep from the PNG. Then we call AddRepresentation to insert it into the map. std::map::insert is doing nothing because kImageRepSkia is already in the map. The only explanation I can think of is that AddRepresentation got called with an ImageRepSkia, in between the call to GetRepresentation and the call to AddRepresentation. (Like, for example, ToImageSkia is running concurrently on two separate threads.) Like I said, I can't see how we can get into this situation in either of these cases. I don't have a repro case, and I can't really write a test for this. But let's assume we got into that situation: we shouldn't use-after-free. I think the correct behaviour is to DCHECK, and in a release build to simply return the pointer to the existing ImageRepSkia in the map. The other option is to CHECK and not bother recovering. == History == It's not clear whether this is a new problem in M50 or if it has been around for ages. I introduced this in r326005 / M44 (https://codereview.chromium.org/1099473003) by converting this code from raw pointers to scoped_ptr. Prior to r326005, two simultaneous calls to ToImageSkia would have resulted in a memory leak instead (the new ImageRepSkia would simply leak as it was not inserted into the map). By converting to scoped_ptr, I changed the behaviour so it now frees the memory, resulting in a use-after-free in this case. So has this been around since M44? Another relevant change is r360461 / M49 (https://codereview.chromium.org/1456703004) when limasdf changed from using ScopedPtrMap to std::map. That changes the implementation of insert, but I don't think the behaviour would change. I can't see anything else that would have changed around this since M44. == Fix == CL (private): https://codereview.chromium.org/1769433002 Any thoughts on this?
,
Mar 4 2016
ImageSkia can be used on multiple threads if configured right (search for comments on threads in the header). Did we recently convert some places that had been using ImageSkia to Image? If so, then I suspect we need to make Image have similar threading abilities.
,
Mar 4 2016
Yeah, using gfx::Image from multiple threads is bad. If this is really caused by race, then your CL may not be safe fix because map::insert isn't thread safe. GetImageCheckSum is for app's favicon right? The crash seems to be happening in tab strip. Is there any case that app's favicon can be used in a tab strip? Can you check if favicon loading for contents is using ToImageSkia from another thread? One way to mitigate is to generate ImageRepSkia when favicon is loaded, although it's not perfect.
,
Mar 4 2016
#5: I tried to make gfx::Image thread-safe in Issue 468010 . It was decided that it was out-of-scope and would introduce performance hits, and we should update all the code that uses it instead. Which is hard because we don't know what code makes this assumption. #6: I'll look at these suggestions.
,
Mar 4 2016
I still think I should submit the proposed CL (maybe make the DCHECK into a CHECK). If it is a race, then as you say (#6) it is still unsafe, but at least we'll convert the use-after-free into a CHECK-fail. Having a DCHECK means we'll continue silently into possible thread safety issues, so having a CHECK would mean we crash (securely).
,
Mar 7 2016
I'm not sure whether to treat this as security (TL;DR, it is a use-after-free crash triggered through a race condition, with no known repro steps). We have a fix for it that hasn't landed and is currently private. Security sheriff: please advise on whether to treat this as a security issue (i.e., whether fix should be merged into M49).
,
Mar 7 2016
Alternative CL: (CHECK instead of DCHECK) https://codereview.chromium.org/1773433002/
,
Mar 7 2016
More thinking. Plausible theory. Both of the above crashes are in fact the same scenario, but executing in a different order. I originally thought the two crashes were unrelated bugs with the same proximate cause. But it looks like both of the stack traces we see come from calling ToImageSkia on a favicon that is shared with an Image belonging to a WebContents. Both of the stack traces we are seeing are executing at the same time on different threads. Whichever one hits image.cc:755 second crashes. Favicon Images live in* NavigationEntryImpls, corresponding to the browser tab of that favicon. * I hesitated to say "are owned by" --- we cannot attribute ownership of Images because they are refcounted, and that is the ultimate problem. Consider the following set of circumstances for a particular WebContents: UI thread: 1. OpenAppShortcutWindow (in chrome/browser/ui/extensions/application_launch.cc) 2. extensions::TabHelper::FromWebContents(tab)->UpdateShortcutOnLoadComplete(); 3. Sets update_shortcut_on_load_complete_ = true; 4. (later) TabHelper::Observe 5. GetApplicationInfo(UPDATE_SHORTCUT); 6. (later) TabHelper::OnDidGetWebApplicationInfo 7. web_app::UpdateShortcutForTabContents(web_contents()); (in web_app_win; Windows only) 8. UpdateShortcutWorker* worker = new UpdateShortcutWorker(web_contents); 9. shortcut_info_ = web_app::GetShortcutInfoForTab(web_contents_); -- gets a reference to the favicon Image 10. worker->Run(); 11. DownloadIcon(); 12. unprocessed_icons_.empty() so no need to download an icon 13. UpdateShortcuts() 14. BrowserThread::PostTask(FILE, ... base::Bind(&UpdateShortcutWorker::UpdateShortcutsOnFileThread...)); Now, consider the following happening at ~the same time for the same WebContents: TASK ONE (from crash/2c1490f000000000) UI thread: 1. Browser::ScheduleUIUpdate 2. TabStripModel::UpdateWebContentsStateAt 3. BrowserTabStripController::TabChangedAt 4. BrowserTabStripController::SetTabDataAt 5. BrowserTabStripController::SetTabRendererDataFromModel 6. favicon::ContentFaviconDriver::FromWebContents -- gets a reference to the favicon Image 7. data->favicon = favicon_driver->GetFavicon().AsImageSkia(); TASK TWO (from crash/f7af02f000000000) FILE thread (continuing on from the PostTask earlier): 1. UpdateShortcutWorker::UpdateShortcutsOnFileThread 2. web_app::internals::CheckAndSaveIcon 3. `anonymous namespace'::ShouldUpdateIcon 4. `anonymous namespace'::GetImageCheckSum 5. gfx::Image::AsBitmap() 6. gfx::Image::ToSkBitmap() 7. gfx::Image::ToImageSkia() So the fundamental problem is that UpdateShortcutWorker gets a favicon from a NavigationEntryImpl and then passes it to another thread, which calls ToImageSkia on it. Images are not thread safe. So I think we fix this by changing UpdateShortcutWorker to take a copy of the image. I'll do that, but now I must rest...
,
Mar 14 2016
I haven't heard anything from security team in a week. I am going to land the fix (which means this will be publicly visible). Please advise whether merge is required.
,
Mar 15 2016
sgtm I also agree that we should land https://codereview.chromium.org/1773433002/ to catch similar errors in more readable way.
,
Mar 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7e551c12da627989bf8f31afd7b671279113d92d commit 7e551c12da627989bf8f31afd7b671279113d92d Author: mgiuca <mgiuca@chromium.org> Date: Tue Mar 15 00:56:57 2016 Fix use-after-free in gfx::Image. ToImageSkia, ToUIImage and ToNSImage would insert an ImageRep into the map, then return the pointer to the ImageRep. If the map already contained a rep of that type, the new rep gets freed and the returned pointer is dangling. Adds a CHECK for this case so it will now crash cleanly. This should not happen, but it is evidently possible. This could mean that ToImageSkia is being called from two threads at the same time (which is bad, because gfx::Image is not thread safe). BUG= 590882 Review URL: https://codereview.chromium.org/1773433002 Cr-Commit-Position: refs/heads/master@{#381141} [modify] https://crrev.com/7e551c12da627989bf8f31afd7b671279113d92d/ui/gfx/image/image.cc [modify] https://crrev.com/7e551c12da627989bf8f31afd7b671279113d92d/ui/gfx/image/image.h
,
Mar 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce72b2661074ea7d583a276248d50e9763df5ede commit ce72b2661074ea7d583a276248d50e9763df5ede Author: mgiuca <mgiuca@chromium.org> Date: Thu Mar 17 00:38:26 2016 gfx::Image: Refactor to avoid assuming scoped_ptr lifetime. Previously, ToImageSkia, ToUIImage and ToNSImage would assume the scoped_ptr they pass to AddRepresentation would remain valid after that method returns. This *should* be the case, but it is poor form to assume it. Now AddRepresentation returns a valid pointer and the callers return that, rather than assuming the scoped_ptr stays alive. BUG= 590882 Review URL: https://codereview.chromium.org/1769433002 Cr-Commit-Position: refs/heads/master@{#381607} [modify] https://crrev.com/ce72b2661074ea7d583a276248d50e9763df5ede/ui/gfx/image/image.cc [modify] https://crrev.com/ce72b2661074ea7d583a276248d50e9763df5ede/ui/gfx/image/image.h
,
Mar 21 2016
Filed follow-up bug Issue 596348 for the thread-safety issue. The use-after-free crash has been fixed by r381141. Requesting a merge of r381141 (7e551c12) to M50. This issue has been around since M44 but now that the fix is public I think we should push it into the next release (potential security issue though no exploit is known).
,
Mar 21 2016
,
Mar 21 2016
Your change meets the bar and is auto-approved for M50 (branch: 2661)
,
Mar 21 2016
Please try to merge your change to M50 branch 2661 asap as we're getting closer to M50 beta candidate cut for this week. Thank you.
,
Mar 22 2016
FYI: I will be OOO until 2016-03-30. Appointing raymes@ to watch over this in case anything breaks and it has to be reverted. The potential breakage is new crashes at ui/gfx/image.cc line 760. A handful of crashes may occur there (which is why we put the CHECK in in the first place; it is theoretically possible to hit this CHECK due to an ongoing race condition bug Issue 596348 ), but if there are significant numbers of crashes then it should be reverted. Thanks!
,
Mar 22 2016
,
Mar 22 2016
I don't know why bugdroid hasn't updated this bug. The merge has landed: https://codereview.chromium.org/1822963002 commit c275eea4617914c9c2479b2514e2d884643a55a2 Author: Matt Giuca <mgiuca@chromium.org> Date: Tue Mar 22 01:06:05 2016 Fix use-after-free in gfx::Image. ToImageSkia, ToUIImage and ToNSImage would insert an ImageRep into the map, then return the pointer to the ImageRep. If the map already contained a rep of that type, the new rep gets freed and the returned pointer is dangling. Adds a CHECK for this case so it will now crash cleanly. This should not happen, but it is evidently possible. This could mean that ToImageSkia is being called from two threads at the same time (which is bad, because gfx::Image is not thread safe). BUG= 590882 Review URL: https://codereview.chromium.org/1773433002 Cr-Commit-Position: refs/heads/master@{#381141} (cherry picked from commit 7e551c12da627989bf8f31afd7b671279113d92d) Review URL: https://codereview.chromium.org/1822963002 . Cr-Commit-Position: refs/branch-heads/2661@{#336} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081}
,
Apr 12 2016
,
Apr 12 2016
,
Apr 12 2016
,
Jun 27 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
,
Jul 14 2017
,
Jul 17 2017
-RVG; there is nothing confidential here and the crash has long been addressed (it can still crash; see Issue 742145 , but not security). |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by sky@chromium.org
, Mar 1 2016I don't think this is a bug in the tabstrip code. Rather I think it's in image. Specifically it looks like Image::ToImageSkia doesn't have a rep so it tries to create one. Image ends up here: rep = scoped_rep.get(); AddRepresentation(std::move(scoped_rep)); } return rep->AsImageRepSkia()->image(); If AddRepresentation fails (because there was already a rep), then rep has been deleted and rep->AsImageRepSkia will trigger this crash.