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

Issue 619147 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

CFI: invalid cast in GetTextInputTypeForView

Project Member Reported by krasin@chromium.org, Jun 10 2016

Issue description

Version: 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.
 

Comment 1 by krasin@chromium.org, Jun 10 2016

I've compiled the test with use_cfi_diag=true, and got some additional clue:

../../content/public/test/text_input_test_utils.cc:194:7: runtime error: control flow integrity check for type 'content::RenderWidgetHostViewBase' failed during base-to-derived cast (vtable address 0xffffc41618710e44)
0xffffc41618710e44: note: invalid vtable
<memory cannot be printed>

Looking more closely...

Comment 2 by krasin@chromium.org, Jun 10 2016

Interestingly, it's a different line of code than without diagnostics.

Comment 3 by krasin@chromium.org, 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

Comment 4 by krasin@chromium.org, Jun 10 2016

Cc: sky@chromium.org
Labels: -Pri-2 Pri-1
Owner: ekaramad@chromium.org
Status: Assigned (was: Untriaged)
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.

Comment 5 by creis@chromium.org, Jun 10 2016

Cc: creis@chromium.org

Comment 6 by creis@chromium.org, 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?
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. 

Comment 8 by krasin@chromium.org, Jun 11 2016

Thank you, Ehsan!
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.

Comment 10 Deleted

Oh, that's sad to hear. Which troubles did you have with building it with is_cfi=true? Any particular error message or something?
ekaramad: This test is still failing. Can you please take a look very soon?
I am working on a local build. Will update soon. Thanks!
FYI, Here is the CL in progress:
https://codereview.chromium.org/2054163003/.
Project Member

Comment 15 by bugdroid1@chromium.org, 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

Comment 16 by creis@chromium.org, Jun 29 2016

Status: Fixed (was: Assigned)
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!
Thank you Ehsan and everyone involved!

Sign in to add a comment