CFI: invalid cast in GetTextInputTypeForView |
||||
Issue descriptionVersion: tip OS: Linux x86-64 What steps will reproduce the problem? (1) Build interactive_ui_tests with Control Flow Integrity enabled: $ gn gen out/gn-cfi '--args=is_cfi=true is_debug=false is_component_build=false symbol_level=1 dcheck_always_on=true' --check $ build/download_gold_plugin.py $ ninja -C out/gn-cfi interactive_ui_tests See more about Control Flow Integrity here: https://www.chromium.org/developers/testing/control-flow-integrity (2) Run the failing test case: $ gdb --args ./out/gn-cfi/interactive_ui_tests --gtest_filter=SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame --no-sandbox --single_process (gdb) r ... Program received signal SIGILL, Illegal instruction. content::GetTextInputTypeForView(content::WebContents*, content::RenderWidgetHostView*, ui::TextInputType*) () at ../../content/public/test/text_input_test_utils.cc:191 191 static_cast<WebContentsImpl*>(web_contents)->GetTextInputManager(); (gdb) bt #0 content::GetTextInputTypeForView(content::WebContents*, content::RenderWidgetHostView*, ui::TextInputType*) () at ../../content/public/test/text_input_test_utils.cc:191 #1 0x00000000006324ce in RunTestOnMainThread () at ../../chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:336 #2 0x00000000006a28a9 in RunTestOnMainThreadLoop () at ../../chrome/test/base/in_process_browser_test.cc:544 #3 0x0000000000b7978c in ProxyRunTestOnMainThreadLoop () at ../../content/public/test/browser_test_base.cc:333 #4 0x00000000006e9dd5 in Run () at ../../base/callback.h:397 #5 PreMainMessageLoopRunImpl () at ../../chrome/browser/chrome_browser_main.cc:1842 #6 0x00000000006e7a0a in PreMainMessageLoopRun () at ../../chrome/browser/chrome_browser_main.cc:1166 #7 0x00000000020d3214 in PreMainMessageLoopRun () at ../../content/browser/browser_main_loop.cc:941 #8 0x00000000024641b7 in Run () at ../../base/callback.h:397 #9 RunAllTasksNow () at ../../content/browser/startup_task_runner.cc:45 #10 0x00000000020d187b in CreateStartupTasks () at ../../content/browser/browser_main_loop.cc:831 #11 0x00000000020d5ebb in Initialize () at ../../content/browser/browser_main_runner.cc:139 #12 0x00000000020cddf2 in BrowserMain () at ../../content/browser/browser_main.cc:42 #13 0x00000000059f849e in RunNamedProcessTypeMain () at ../../content/app/content_main_runner.cc:420 #14 0x00000000059f8f4a in Run () at ../../content/app/content_main_runner.cc:787 #15 0x00000000059f75da in ContentMain () at ../../content/app/content_main.cc:20 #16 0x0000000000b7908c in SetUp () at ../../content/public/test/browser_test_base.cc:306 #17 0x00000000006a2384 in SetUp () at ../../chrome/test/base/in_process_browser_test.cc:238 #18 0x0000000005afe44b in HandleExceptionsInMethodIfSupported<testing::Test, void> () at ../../testing/gtest/src/gtest.cc:2458 #19 Run () at ../../testing/gtest/src/gtest.cc:2470 #20 0x0000000005aff7e5 in Run () at ../../testing/gtest/src/gtest.cc:2656 #21 0x0000000005affbb3 in Run () at ../../testing/gtest/src/gtest.cc:2774 #22 0x0000000005b043e9 in RunAllTests () at ../../testing/gtest/src/gtest.cc:4647 #23 0x0000000005b0401f in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> () at ../../testing/gtest/src/gtest.cc:2458 #24 Run () at ../../testing/gtest/src/gtest.cc:4255 #25 0x000000000127f996 in RUN_ALL_TESTS () at ../../testing/gtest/include/gtest/gtest.h:2237 #26 Run () at ../../base/test/test_suite.cc:230 #27 0x0000000000589734 in RunTestSuite () at ../../chrome/test/base/interactive_ui_tests_main.cc:74 #28 0x0000000005a023b8 in LaunchTests () at ../../content/public/test/test_launcher.cc:517 #29 0x00000000005896bd in main () at ../../chrome/test/base/interactive_ui_tests_main.cc:86 'CFI Linux' and 'CFI Linux ToT' bots are broken due to this bug. UBSan Vptr does not catch it, unfortunately.
,
Jun 10 2016
Interestingly, it's a different line of code than without diagnostics.
,
Jun 10 2016
It also has a more detailed stack trace: breakpoint 1, 0x0000000000474a30 in __ubsan::HandleCFIBadType(__ubsan::CFICheckFailData*, unsigned long, bool, __ubsan::ReportOptions) () (gdb) bt #0 0x0000000000474a30 in __ubsan::HandleCFIBadType(__ubsan::CFICheckFailData*, unsigned long, bool, __ubsan::ReportOptions) () #1 0x0000000000473afb in __ubsan_handle_cfi_check_fail () #2 0x000000000660cdd5 in content::GetTextInputTypeForView(content::WebContents*, content::RenderWidgetHostView*, ui::TextInputType*) () at ../../content/public/test/text_input_test_utils.cc:194 #3 0x00000000005cb8c1 in SitePerProcessTextInputManagerTest_StopTrackingCrashedChildFrame_Test::RunTestOnMainThread() () at ../../chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc:336 #4 0x0000000000624e99 in InProcessBrowserTest::RunTestOnMainThreadLoop() () at ../../chrome/test/base/in_process_browser_test.cc:544 #5 0x0000000000d0ebaa in content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() () at ../../content/public/test/browser_test_base.cc:333 #6 0x000000000068b5ed in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() () at ../../chrome/browser/chrome_browser_main.cc:1842 #7 0x00000000006887f6 in ChromeBrowserMainParts::PreMainMessageLoopRun() () at ../../chrome/browser/chrome_browser_main.cc:1166 #8 0x000000000257a96d in content::BrowserMainLoop::PreMainMessageLoopRun() () at ../../content/browser/browser_main_loop.cc:941 #9 0x000000000257b62b in base::internal::Invoker<base::IndexSequence<0ul>, base::internal::BindState<base::internal::RunnableAdapter<int (content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*), base::internal::UnretainedWrapper<content::BrowserMainLoop> >, false, int ()>::Run(base::internal::BindStateBase*) () at ../../base/bind_internal.h:364 #10 0x0000000002aa7170 in content::StartupTaskRunner::RunAllTasksNow() () at ../../content/browser/startup_task_runner.cc:45 #11 0x00000000025798e2 in content::BrowserMainLoop::CreateStartupTasks() () at ../../content/browser/browser_main_loop.cc:831 #12 0x000000000257d46e in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) () at ../../content/browser/browser_main_runner.cc:139 #13 0x0000000002574571 in content::BrowserMain(content::MainFunctionParams const&) () at ../../content/browser/browser_main.cc:42 #14 0x00000000065fd66e in content::RunNamedProcessTypeMain(std::string const&, content::MainFunctionParams const&, content::ContentMainDelegate*) () at ../../content/app/content_main_runner.cc:420 #15 0x00000000065fe20f in content::ContentMainRunnerImpl::Run() () at ../../content/app/content_main_runner.cc:787 #16 0x00000000065fbfe5 in content::ContentMain(content::ContentMainParams const&) () at ../../content/app/content_main.cc:20 #17 0x0000000000d0e7de in content::BrowserTestBase::SetUp() () at ../../content/public/test/browser_test_base.cc:306 #18 0x0000000000624b16 in InProcessBrowserTest::SetUp() () at ../../chrome/test/base/in_process_browser_test.cc:238 #19 0x000000000673f8c5 in testing::Test::Run() () at ../../testing/gtest/src/gtest.cc:2470 #20 0x0000000006741466 in testing::TestInfo::Run() () at ../../testing/gtest/src/gtest.cc:2656 #21 0x0000000006741782 in testing::TestCase::Run() () at ../../testing/gtest/src/gtest.cc:2774 #22 0x00000000067444a2 in testing::internal::UnitTestImpl::RunAllTests() () at ../../testing/gtest/src/gtest.cc:4647 #23 0x00000000067446a9 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) () at ../../testing/gtest/src/gtest.cc:2455 #24 0x000000000674404b in testing::UnitTest::Run() () at ../../testing/gtest/src/gtest.cc:4255 #25 0x0000000001560511 in base::TestSuite::Run() () at ../../base/test/test_suite.cc:230 #26 0x0000000000541cac in InteractiveUITestSuiteRunner::RunTestSuite(int, char**) () at ../../chrome/test/base/interactive_ui_tests_main.cc:74 #27 0x0000000006607dc3 in content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) () at ../../content/public/test/test_launcher.cc:517 #28 0x0000000000541c35 in main () at ../../chrome/test/base/interactive_ui_tests_main.cc:86
,
Jun 10 2016
Oh, here is gold: https://cs.chromium.org/chromium/src/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc?q=site_per_process_text_input_browsertest.cc:336&sq=package:chromium&l=336 // Verifying that the TextInputManager is no longer tracking TextInputState // for |first_view|. Note that |first_view| is now a dangling pointer. EXPECT_FALSE(content::GetTextInputTypeForView(active_contents(), first_view, &first_view_type)); Since first_view points to a garbage, and then there's a cast, CFI correctly detects a violation. This was introduced in https://codereview.chromium.org/1948343002. Assigning the bug to the author of the test.
,
Jun 10 2016
,
Jun 10 2016
I think the test just uses the stale pointer as a key into a map and doesn't dereference it, but it's not a good pattern and is open to future bugs. Ehsan, can you update the test to avoid it?
,
Jun 11 2016
Yes, as creis@ said in C#6 the test is just using the pointer as the key in a map. I am working on a CL to change this.
,
Jun 11 2016
Thank you, Ehsan!
,
Jun 11 2016
You're welcome. Here is the CL FYI: https://codereview.chromium.org/2054163003/. Also, I have not been able to build with 'is_cfi=true', yet. But I assume the fix should work since there is no usage of stale pointers altogether.
,
Jun 11 2016
Oh, that's sad to hear. Which troubles did you have with building it with is_cfi=true? Any particular error message or something?
,
Jun 24 2016
ekaramad: This test is still failing. Can you please take a look very soon?
,
Jun 24 2016
I am working on a local build. Will update soon. Thanks!
,
Jun 24 2016
FYI, Here is the CL in progress: https://codereview.chromium.org/2054163003/.
,
Jun 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/936536dbbfd6cb9cf468c80aba316cabafb0f002 commit 936536dbbfd6cb9cf468c80aba316cabafb0f002 Author: ekaramad <ekaramad@chromium.org> Date: Wed Jun 29 05:44:44 2016 Fix SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame on CFI Bots. Currently, the test SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame uses stale pointers of the views agains a map to verify that TextInputManager does not track the view anymore. This is causing issues in Linux CFI bots. This CL changes the test so that we now check for the number of views being tracked and verify that after crashing a frame, the number drops by one. BUG= 619147 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2054163003 Cr-Commit-Position: refs/heads/master@{#402736} [modify] https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc [modify] https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002/content/browser/renderer_host/text_input_manager.cc [modify] https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002/content/browser/renderer_host/text_input_manager.h [modify] https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002/content/public/test/text_input_test_utils.cc [modify] https://crrev.com/936536dbbfd6cb9cf468c80aba316cabafb0f002/content/public/test/text_input_test_utils.h
,
Jun 29 2016
It looks like SitePerProcessTextInputManagerTest.StopTrackingCrashedChildFrame has stopped failing on https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux after build 5919, so I think we can mark this as fixed. Thanks!
,
Jun 29 2016
Thank you Ehsan and everyone involved! |
||||
►
Sign in to add a comment |
||||
Comment 1 by krasin@chromium.org
, Jun 10 2016