Deploy use-after-scope check on ASAN bots |
|||
Issue descriptionRecently, I have unproductively spent 3 days on a "simple" use-after-scope bug: https://codereview.chromium.org/2354333008/ This was detected by unrelated bot (ThinLTO Linux ToT) and a lot of effort went into realizing that it's not ThinLTO or Clang to blame. We need to deploy -fsanitize-address-use-after-scope check in Chrome, and this is a tracking issue for that.
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7e7cb2767b4aa6811b8c202d8cce97ab020c4174 commit 7e7cb2767b4aa6811b8c202d8cce97ab020c4174 Author: krasin <krasin@chromium.org> Date: Thu Oct 27 22:06:51 2016 Fix use-after-scope bug in IntrusiveHeapTest.HeapIndex. The heap object contains references to the handles, and its destructor must run before they go out of scope. This is caught with -fsanitize=address -fsanitize-address-use-after-scope. See https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/251354/steps/blink_platform_unittests%20%28with%20patch%29%20on%20Ubuntu-12.04/logs/stdio BUG= 649897 Review-Url: https://codereview.chromium.org/2453013004 Cr-Commit-Position: refs/heads/master@{#428152} [modify] https://crrev.com/7e7cb2767b4aa6811b8c202d8cce97ab020c4174/third_party/WebKit/Source/platform/scheduler/base/intrusive_heap_unittest.cc
,
Dec 7 2016
The current state is that a lot of browser_tests fails with reports about a bug in libpng: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/274610/steps/browser_tests%20%28with%20patch%29/logs/stdio ================================================================= ==1150==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f630a30ff20 at pc 0x000002c51bec bp 0x7f630b1dbda0 sp 0x7f630b1dbd98 READ of size 8 at 0x7f630a30ff20 thread T14 (Chrome_FileThre) #0 0x2c51beb in cr_png_destroy_info_struct third_party/libpng/png.c:376:18 #1 0x2c8ff7e in cr_png_destroy_write_struct third_party/libpng/pngwrite.c:978:10 #2 0xc0825e4 in ~PngWriteStructDestroyer ui/gfx/codec/png_codec.cc:338:5 #3 0xc0825e4 in gfx::(anonymous namespace)::EncodeWithCompressionLevel(unsigned char const*, gfx::PNGCodec::ColorFormat, gfx::Size const&, int, bool, std::__1::vector<gfx::PNGCodec::Comment, std::__1::allocator<gfx::PNGCodec::Comment> > const&, int, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >*) ui/gfx/codec/png_codec.cc:730 #4 0xc082a97 in gfx::(anonymous namespace)::InternalEncodeSkBitmap(SkBitmap const&, bool, int, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >*) ui/gfx/codec/png_codec.cc:745:10 #5 0xc0aa900 in gfx::internal::Get1xPNGBytesFromImageSkia(gfx::ImageSkia const*) ui/gfx/image/image.cc:159:8 #6 0xc0ae033 in gfx::Image::As1xPNGBytes() const ui/gfx/image/image.cc:621:19 #7 0x8dc269d in CreateShortcutIcon chrome/browser/shell_integration_linux.cc:294:58 #8 0x8dc269d in shell_integration_linux::CreateDesktopShortcut(web_app::ShortcutInfo const&, web_app::ShortcutLocations const&) chrome/browser/shell_integration_linux.cc:964 #9 0x1722ac5f in web_app::internals::CreatePlatformShortcuts(base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason) chrome/browser/web_applications/web_app_linux.cc:32:10 #10 0xf322568 in bool base::internal::FunctorTraits<bool (*)(base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason), void>::Invoke<base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason const&>(bool (*)(base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason), base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >&&, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason const&) base/bind_internal.h:164:12 #11 0xf3222d5 in Invoke<const base::internal::IgnoreResultHelper<bool (*)(const base::FilePath &, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, const std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > &, const web_app::ShortcutLocations &, web_app::ShortcutCreationReason)> &, const base::FilePath &, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, const std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > &, const web_app::ShortcutLocations &, const web_app::ShortcutCreationReason &> base/bind_internal.h:247:5 #12 0xf3222d5 in MakeItSo<const base::internal::IgnoreResultHelper<bool (*)(const base::FilePath &, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, const std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > &, const web_app::ShortcutLocations &, web_app::ShortcutCreationReason)> &, const base::FilePath &, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, const std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > &, const web_app::ShortcutLocations &, const web_app::ShortcutCreationReason &> base/bind_internal.h:285 #13 0xf3222d5 in void base::internal::Invoker<base::internal::BindState<base::internal::IgnoreResultHelper<bool (*)(base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason)>, base::FilePath, base::internal::PassedWrapper<std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> > >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> >, web_app::ShortcutLocations, web_app::ShortcutCreationReason>, void ()>::RunImpl<base::internal::IgnoreResultHelper<bool (*)(base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason)> const&, std::__1::tuple<base::FilePath, base::internal::PassedWrapper<std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> > >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> >, web_app::ShortcutLocations, web_app::ShortcutCreationReason> const&, 0ul, 1ul, 2ul, 3ul, 4ul>(base::internal::IgnoreResultHelper<bool (*)(base::FilePath const&, std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> > const&, web_app::ShortcutLocations const&, web_app::ShortcutCreationReason)> const&, std::__1::tuple<base::FilePath, base::internal::PassedWrapper<std::__1::unique_ptr<web_app::ShortcutInfo, std::__1::default_delete<web_app::ShortcutInfo> > >, std::__1::vector<extensions::FileHandlerInfo, std::__1::allocator<extensions::FileHandlerInfo> >, web_app::ShortcutLocations, web_app::ShortcutCreationReason> const&, base::IndexSequence<0ul, 1ul, 2ul, 3ul, 4ul>) base/bind_internal.h:361 #14 0x845ed1b in Run base/callback.h:68:12 #15 0x845ed1b in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) base/debug/task_annotator.cc:52 #16 0x82ac37a in base::MessageLoop::RunTask(base::PendingTask*) base/message_loop/message_loop.cc:413:19 #17 0x82ad325 in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) base/message_loop/message_loop.cc:422:5 #18 0x82ae8fd in base::MessageLoop::DoWork() base/message_loop/message_loop.cc:515:13 #19 0x82bd5c0 in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_libevent.cc:218:31 #20 0x82aba2a in base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:378:10 #21 0x8338ebe in base::RunLoop::Run() base/run_loop.cc:35:10 #22 0x83bafdb in base::Thread::Run(base::RunLoop*) base/threading/thread.cc:245:13 #23 0x5ee61c4 in content::BrowserThreadImpl::FileThreadRun(base::RunLoop*) content/browser/browser_thread_impl.cc:227:11 #24 0x5ee6f3b in content::BrowserThreadImpl::Run(base::RunLoop*) content/browser/browser_thread_impl.cc:280:14 #25 0x83bbbfd in base::Thread::ThreadMain() base/threading/thread.cc:328:3 #26 0x83a54c2 in base::(anonymous namespace)::ThreadFunc(void*) base/threading/platform_thread_posix.cc:71:13 #27 0x7f6328a50e99 in start_thread /build/eglibc-oqps9y/eglibc-2.15/nptl/pthread_create.c:308 Address 0x7f630a30ff20 is located in stack of thread T14 (Chrome_FileThre) at offset 800 in frame #0 0xc08217f in gfx::(anonymous namespace)::EncodeWithCompressionLevel(unsigned char const*, gfx::PNGCodec::ColorFormat, gfx::Size const&, int, bool, std::__1::vector<gfx::PNGCodec::Comment, std::__1::allocator<gfx::PNGCodec::Comment> > const&, int, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >*) ui/gfx/codec/png_codec.cc:636 This frame has 5 object(s): [32, 328) 'temp.lvalue' [400, 696) 'temp.lvalue24' [768, 776) 'png_ptr:711' [800, 808) 'info_ptr:716' <== Memory access at offset 800 is inside this variable [832, 840) 'state:723' HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext (longjmp and C++ exceptions *are* supported) Thread T14 (Chrome_FileThre) created by T0 (browser_tests) here: #0 0x6c1586 in __interceptor_pthread_create (/b/swarm_slave/w/irK7XBxW/out/Release/browser_tests+0x6c1586) #1 0x83a4611 in base::(anonymous namespace)::CreateThread(unsigned long, bool, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:110:13 #2 0x83b9f05 in base::Thread::StartWithOptions(base::Thread::Options const&) base/threading/thread.cc:112:15 #3 0x5ee7bcf in content::BrowserThreadImpl::StartWithOptions(base::Thread::Options const&) content/browser/browser_thread_impl.cc:359:25 #4 0x5eb8919 in content::BrowserMainLoop::CreateThreads() content/browser/browser_main_loop.cc:956:32 #5 0x6caca69 in Run base/callback.h:85:12 #6 0x6caca69 in content::StartupTaskRunner::RunAllTasksNow() content/browser/startup_task_runner.cc:45 #7 0x5eb7d69 in content::BrowserMainLoop::CreateStartupTasks() content/browser/browser_main_loop.cc:864:25 #8 0x5ec652d in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) content/browser/browser_main_runner.cc:126:17 #9 0x5eafcf3 in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:42:32 #10 0x7fb6fca in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:416:14 #11 0x7fb8e1d in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:786:12 #12 0x7fb4f9a in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:20:28 #13 0x9923c55 in content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:316:3 #14 0x84eb91f in InProcessBrowserTest::SetUp() chrome/test/base/in_process_browser_test.cc:251:20 #15 0xb2f52b6 in HandleExceptionsInMethodIfSupported<testing::Test, void> testing/gtest/src/gtest.cc:2458:12 #16 0xb2f52b6 in testing::Test::Run() testing/gtest/src/gtest.cc:2470 #17 0xb2f7154 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:11 #18 0xb2f8416 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:28 #19 0xb30dfe6 in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:43 #20 0xb30d4c5 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 #21 0xb30d4c5 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255 #22 0x85157a6 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46 #23 0x85157a6 in base::TestSuite::Run() base/test/test_suite.cc:271 #24 0x8210953 in ChromeTestSuiteRunner::RunTestSuite(int, char**) chrome/test/base/chrome_test_launcher.cc:64:38 #25 0x99f72d9 in content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) content/public/test/test_launcher.cc:520:31 #26 0x8210791 in main chrome/test/base/browser_tests_main.cc:15:10 #27 0x7f6323bb07ec in __libc_start_main /build/eglibc-oqps9y/eglibc-2.15/csu/libc-start.c:226 SUMMARY: AddressSanitizer: stack-use-after-scope third_party/libpng/png.c:376:18 in cr_png_destroy_info_struct Shadow bytes around the buggy address: 0x0fece1459f90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fece1459fa0: 00 00 00 00 00 00 00 00 00 f2 f2 f2 f2 f2 f2 f2 0x0fece1459fb0: f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fece1459fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fece1459fd0: 00 00 00 00 00 00 00 f2 f2 f2 f2 f2 f2 f2 f2 f2 =>0x0fece1459fe0: 00 f2 f2 f2[f8]f2 f2 f2 f8 f3 f3 f3 00 00 00 00 0x0fece1459ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fece145a000: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 0x0fece145a010: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 0x0fece145a020: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 0x0fece145a030: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb
,
Dec 7 2016
I am going to take a look at this at the appropriate priority (Pri=3).
,
Dec 14 2016
The fix is out for a review: https://codereview.chromium.org/2576823002/#
,
Dec 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/23478bc8eb2dcf7df4678359e8fb164a5317313f commit 23478bc8eb2dcf7df4678359e8fb164a5317313f Author: krasin <krasin@chromium.org> Date: Thu Dec 15 23:39:36 2016 Fix stack-use-after-scope in png_codec. PngWriteStructDestroyer accesses info_ptr, so info_ptr must have a wider lifetime scope. Otherwise, the behavior of the problem is undefined. The issue is found by AddressSanitizer with use-after-scope check enabled. BUG= 649897 Review-Url: https://codereview.chromium.org/2576823002 Cr-Commit-Position: refs/heads/master@{#438954} [modify] https://crrev.com/23478bc8eb2dcf7df4678359e8fb164a5317313f/ui/gfx/codec/png_codec.cc
,
Dec 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/360dee282db84d1e63414c9e8d471a7465a8441d commit 360dee282db84d1e63414c9e8d471a7465a8441d Author: krasin <krasin@chromium.org> Date: Tue Dec 20 22:07:03 2016 Fix use-after-free in TextureLayerImplTest.ResourceNotFreedOnGpuRasterToggle. The callback uses a local variable 'released', but 'released' had a more narrow scope than the owner of the callback (and it's called from the destructor, when 'released' is already out of scope) BUG= 649897 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2596593003 Cr-Commit-Position: refs/heads/master@{#439888} [modify] https://crrev.com/360dee282db84d1e63414c9e8d471a7465a8441d/cc/layers/texture_layer_impl_unittest.cc
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9416e5d3947a57e2217a8aa63535f9cf9c4ca15 commit e9416e5d3947a57e2217a8aa63535f9cf9c4ca15 Author: krasin <krasin@chromium.org> Date: Wed Dec 21 01:10:35 2016 Fix use-after-free issue in VideoDecoderResourceTest. Local variables responsible for shared memory go out of scope right after their initialization and then accessed by the saved references to the stack space which is already potentially reused for other local variables. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 TBR=piman@chromium.org Review-Url: https://codereview.chromium.org/2593873002 Cr-Commit-Position: refs/heads/master@{#439961} [modify] https://crrev.com/e9416e5d3947a57e2217a8aa63535f9cf9c4ca15/ppapi/proxy/video_decoder_resource_unittest.cc
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f21d6e5f8417163e52730a30b9e85fb6cfd6f251 commit f21d6e5f8417163e52730a30b9e85fb6cfd6f251 Author: krasin <krasin@chromium.org> Date: Wed Dec 21 02:21:06 2016 Fix use-after-scope issue in WebFrameTest. Local variable client goes out of scope before WebViewHelper, which indirectly access client during destruction. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 Review-Url: https://codereview.chromium.org/2591733003 Cr-Commit-Position: refs/heads/master@{#439982} [modify] https://crrev.com/f21d6e5f8417163e52730a30b9e85fb6cfd6f251/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c06a72a8cb9d22ed8606f1a48ec881aeeb1f7891 commit c06a72a8cb9d22ed8606f1a48ec881aeeb1f7891 Author: krasin <krasin@chromium.org> Date: Wed Dec 21 03:42:46 2016 Fix use-after-scope issues in HttpNetworkTransactionTest. Network transaction keeps a reference to the request and accesses it from the destructor. That adds a requirement that the life scope of request must be wider than the scope of transaction. This CL fixes the cases where it was not true. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 Review-Url: https://codereview.chromium.org/2589363002 Cr-Commit-Position: refs/heads/master@{#440000} [modify] https://crrev.com/c06a72a8cb9d22ed8606f1a48ec881aeeb1f7891/net/http/http_network_transaction_unittest.cc
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f51080b57ed7b3570a7a890b6ad5ef4ddf49747e commit f51080b57ed7b3570a7a890b6ad5ef4ddf49747e Author: krasin <krasin@chromium.org> Date: Wed Dec 21 03:43:38 2016 Fix use-after-free in WidgetCaptureTest.SetCaptureToNonToplevel. This test case calls AddObserver with a pointer to a local variable. The observer is then called during the test instance destruction, when the local variable is already out of scope. The fix is to call RemoveObserver while the observer variable is still alive. The bug was found by AddressSanitizer with use-after-scope check enabled. BUG= 649897 Review-Url: https://codereview.chromium.org/2593613003 Cr-Commit-Position: refs/heads/master@{#440001} [modify] https://crrev.com/f51080b57ed7b3570a7a890b6ad5ef4ddf49747e/ui/views/widget/widget_interactive_uitest.cc
,
Dec 21 2016
Today, I have fixed a number of preexisting use-after-scope issues. Two more CLs are under review: https://codereview.chromium.org/2591913002/ https://codereview.chromium.org/2596613002/ A couple of more fixes are still needed. After that, it will be possible to enable use-after-scope on ASAN bots. Hopefully, it will be done tomorrow.
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9e8006ae521235d3e9bd0f05f3b445ac37066e40 commit 9e8006ae521235d3e9bd0f05f3b445ac37066e40 Author: krasin <krasin@chromium.org> Date: Wed Dec 21 04:13:55 2016 Fix use-after-scope issues with incorrect usage of ManifestParser. ManifestParser accepts references to StringPiece and GURL. In one case, the test passed inline values which went out of scope immediately, and then the parser used the references pointing to a stack space potentially already reused by other local variables. In other case, string was implicitly converted to StringPiece which also went out of scope before Parse call. Both issues are due to the constructor taking references. It's dangerous, and potentially should be changed. The issue was found by AddressSanitizer with use-after-scope check enabled. BUG= 649897 TBR=mlamouri@chromium.org Review-Url: https://codereview.chromium.org/2596613002 Cr-Commit-Position: refs/heads/master@{#440010} [modify] https://crrev.com/9e8006ae521235d3e9bd0f05f3b445ac37066e40/content/renderer/manifest/manifest_manager.cc [modify] https://crrev.com/9e8006ae521235d3e9bd0f05f3b445ac37066e40/content/renderer/manifest/manifest_parser_unittest.cc
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b7bb99fe5dfd71c08bbe4dfa23a1cce39929af22 commit b7bb99fe5dfd71c08bbe4dfa23a1cce39929af22 Author: krasin <krasin@chromium.org> Date: Wed Dec 21 04:35:41 2016 Fix use-after-free issues in UpgradeDetectorImplTest. UpgradeDetectorImpl posts a task to check for unstable channel onto FILE thread. We need to give this task to be executed before the detector goes out of scope. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 Review-Url: https://codereview.chromium.org/2591913002 Cr-Commit-Position: refs/heads/master@{#440016} [modify] https://crrev.com/b7bb99fe5dfd71c08bbe4dfa23a1cce39929af22/chrome/browser/upgrade_detector_impl_unittest.cc
,
Dec 21 2016
All fixes from today have been committed (a couple of them with TBR; but they are trivial and also fix a memory corruption issue, so these TBRs feel justified). I have sent try jobs for https://codereview.chromium.org/2451973004/ to see how many failures remain in the tree.
,
Dec 21 2016
Still failing tests: browser_tests: ExtensionContextMenuBrowserTest.LongTitle AutocompleteBrowserTest.FocusSearch gfx_unittests: TextEliderTest.TruncateString net_unittests: SpdyNetworkTransactionTest.OutOfOrderHeaders SpdyNetworkTransactionTest.ThreeGets URLRequestTestHTTP.URLRequestDelegateOnRedirectCancelled HttpNetworkTransactionSSLTest.TokenBindingAsync SpdyNetworkTransactionTest.StartTransactionOnReadCallback
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ebc675de6100792906c33c0e0be5ba8c78940f3d commit ebc675de6100792906c33c0e0be5ba8c78940f3d Author: krasin <krasin@chromium.org> Date: Wed Dec 21 23:36:09 2016 Fix use-after-scope bug in TextElider. string.c_str() result was implicitly converted to icu::UnicodeString, a reference to the temp variable was saved (and then used) by icu::BreakIterator at the time when the stack address was already potentially being reused for other local variables. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 Review-Url: https://codereview.chromium.org/2591023004 Cr-Commit-Position: refs/heads/master@{#440263} [modify] https://crrev.com/ebc675de6100792906c33c0e0be5ba8c78940f3d/ui/gfx/text_elider.cc
,
Dec 22 2016
I have started a new try job, and it's my hope that only a few net_unittests cases remain unfixed. Their time will come in January (with all potential regressions, which should not be many)
,
Dec 22 2016
Correct, it's just net_unittests: SpdyNetworkTransactionTest.OutOfOrderHeaders SpdyNetworkTransactionTest.ThreeGets URLRequestTestHTTP.URLRequestDelegateOnRedirectCancelled HttpNetworkTransactionSSLTest.TokenBindingAsync SpdyNetworkTransactionTest.StartTransactionOnReadCallback
,
Jan 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0bfeb6b121c5cfd7658c485dcf2077826bad2f2b commit 0bfeb6b121c5cfd7658c485dcf2077826bad2f2b Author: krasin <krasin@chromium.org> Date: Fri Jan 13 21:48:04 2017 net: fix multiple use-after-scope issues in net_unittests. Most of the issues are due to the fact that Transaction destructor accesses the request reference that is stack allocated and has a shorter lifespan, which causes an invalid access to a potentially reused memory. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 Review-Url: https://codereview.chromium.org/2635533002 Cr-Commit-Position: refs/heads/master@{#443685} [modify] https://crrev.com/0bfeb6b121c5cfd7658c485dcf2077826bad2f2b/net/http/http_network_transaction_ssl_unittest.cc [modify] https://crrev.com/0bfeb6b121c5cfd7658c485dcf2077826bad2f2b/net/spdy/spdy_network_transaction_unittest.cc [modify] https://crrev.com/0bfeb6b121c5cfd7658c485dcf2077826bad2f2b/net/url_request/url_request_unittest.cc
,
Jan 13 2017
At this point, Chromium code base is use-after-scope free, at least, I am not aware of any existing failures. I will prepare a CL to enable use-after-scope by default and hope to submit it once it gets approved.
,
Jan 17 2017
Thank you very much for your work on this.
,
Jan 17 2017
So, I sent a few try jobs to https://codereview.chromium.org/2451973004/ on Friday. Green: linux_chromium_asan_rel_ng (this is what I was mostly fixing) win_chromium_syzyasan_rel Red: mac_chromium_asan_rel_ng, https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_asan_rel_ng/builds/4434 Failing browser_tests: CardUnmaskPromptViewBrowserTest.InvokeDialog_expired CollectedCookiesTestMd.InvokeDialog_default CardUnmaskPromptViewBrowserTest.InvokeDialog_valid AppControllerMainMenuBrowserTest.BookmarksMenuIsRestoredAfterProfileSwitch AppShimMenuControllerBrowserTest.PlatformAppFocusUpdatesMenuBar ProfileManagerBrowserTest.EphemeralProfile AppControllerMainMenuBrowserTest.HistoryMenuResetAfterProfileDeletion Failing interactive_ui_tests: OmniboxViewTest.AcceptKeywordByTypingQuestionMark OmniboxViewTest.PreserveDisplayTextOnFocusSearch AppWindowInteractiveTest.ESCLeavesFullscreenDOM CommandsApiTest.RemoveBookmarkShortcutWithoutPermission AppWindowInteractiveTest.ESCLeavesFullscreenWindow OmniboxViewTest.CutURLToClipboard OmniboxViewTest.DeleteItem WebViewContextMenuInteractiveTest.ContextMenuParamCoordinates WebViewPopupInteractiveTest.PopupPositioningMoved WebViewInteractiveTests/WebViewNewWindowInteractiveTest.NewWindow_DiscardAfterOpenerDestroyed/1 WebViewInteractiveTests/WebViewNewWindowInteractiveTest.NewWindow_DiscardAfterOpenerDestroyed/0 CommandsApiTest.OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding OmniboxViewTest.EditSearchEngines FlashFullscreenInteractiveBrowserTest.FullscreenFromSubframe (and many others) Failing unit_tests: LayoutTest.SkipColumns HistoryMenuBridgeTest.ClearHistoryMenuUntilEnd LayoutTest.SimpleGridLayoutHeightForWidth LayoutTest.GetLastValidColumnSet LayoutTest.GridLayoutAddGetColumnSet LayoutTest.AddRowAutogeneratesColumnSetId HistoryMenuBridgeTest.ClearHistoryMenuSkipping LayoutTest.SkipPaddingColumns LayoutTest.AddPaddingRow LayoutTest.SimpleGridLayoutAdjustsViews LayoutTest.AddRowCreatesNewColumnSet LayoutTest.SizeColumnsAdjustLocationAndSize LayoutTest.PreferredHeightForEmptyLayout LayoutTest.AddRowResetsNextColumn LayoutTest.ColumnLayoutAdjustsViews LayoutTest.AddViewSkipsPaddingColumns BookmarkBarFolderControllerTest.DropDestination BookmarkBarControllerTest.DropDestination LayoutTest.SizeRowsAdjustsRowLocations LayoutTest.RowsAccomodateHeightOfAllElements linux_chromium_chromeos_asan_rel_ng: Failing ash_content_unittests: ShelfAlignmentAny_KeyboardOverlayDelegateTest.ShowAndClose_0 ShelfAlignmentAny_KeyboardOverlayDelegateTest.ShowAndClose_1 ShelfAlignmentAny_KeyboardOverlayDelegateTest.ShowAndClose_2 ShelfAlignmentAny_KeyboardOverlayDelegateTest.ShowAndClose_3 Failing ash_unittests: AshPopupAlignmentDelegateTest.LockScreen AshPopupAlignmentDelegateTest.ShelfAlignment FrameSizeButtonTest.ResetButtonsAfterClick FrameSizeButtonTest.SizeButtonPressedWhenSnapButtonHovered Failing browser_tests: AccountsOptionsTest.PRE_MultiProfilesAccountsOptions BlockingLoginTestInstance_BlockingLoginTest.LoginBlocksForUser_0 BluetoothHostPairingNoInputTest.BluetoothDeviceConnected BluetoothHostPairingNoInputTest.NoBluetoothDeviceConnected BluetoothHostPairingWithInputTest.BluetoothDisableByDefault BootstrapTest.Basic BootstrapTest.PRE_CleanUpFailedUser BrailleDisplayPrivateAPIUserTest.KeyEventOnLockScreen BrowserLoginTest.PRE_BrowserActive CaptivePortalAuthenticationIgnoresProxy_NetworkPortalDetectorImplBrowserTestIgnoreProxy.PRE_TestWithPreference_0 CaptivePortalAuthenticationIgnoresProxy_NetworkPortalDetectorImplBrowserTestIgnoreProxy.PRE_TestWithPreference_1 CaptivePortalWindowCtorDtorTest.PRE_OpenPortalDialog CertificateManagerDialogWebUITest.Basic CheckSystemTokenAvailability_EnterprisePlatformKeysTest.Basic_1 CheckSystemTokenAvailability_EnterprisePlatformKeysTest.Basic_2 ChromeSessionManagerTest.OobeNewUser ChromeSessionManagerTest.PRE_LoginExistingUsers DemoAppLauncherTest.Basic DemoAppLauncherTest.NoNetwork (and many others) Failing interactive_ui_tests: LoginSigninTest.WebUIVisible LoginStateNotificationBlockerChromeOSBrowserTest.PRE_AlwaysAllowedNotifier LoginStateNotificationBlockerChromeOSBrowserTest.PRE_BaseTest LoginTest.PRE_GaiaAuthOffline LoginUITest.OobeCatchException LoginUITest.OobeNoExceptions LoginUITest.PRE_InterruptedAutoStartEnrollment LoginUITest.PRE_LoginNoExceptions LoginUITest.PRE_LoginUIVisible OobeTest.Accelerator OobeTest.NewUser ScreenLockerTest.TestBasic ScreenLockerTest.TestFullscreenExit ScreenLockerTest.TestShowTwice Such a drastic difference between linux, chromeos and mac bots comes as a surprise to me. I will take a closer look.
,
Jan 18 2017
ash_unittests failures fixed in https://codereview.chromium.org/2643853003/# (under review)
,
Jan 19 2017
- ash_content_unittests are now covered by https://codereview.chromium.org/2643853003 as well. - a fix for interactive_ui_tests is https://codereview.chromium.org/2643943003/ - I am looking into browser_tests Once the fixes for those are submitted, I will enable the use-after-scope check for all OSes except Mac, and then continue with Mac-specific fixes.
,
Jan 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d377464c5d3b7aaa13823000932c1a23f3c4d01c commit d377464c5d3b7aaa13823000932c1a23f3c4d01c Author: krasin <krasin@chromium.org> Date: Thu Jan 19 19:14:22 2017 Fix stack-use-after-scope in ChromeOS OOBE handler. What happens here (before the fix) is the following series of events: 1. display::Screen::GetScreen() returns Screen* pointer 2. Then GetPrimaryDisplay() returns Display value. This value is stored on the stack and has the scope of just this line of the code. After that the compiler is free to reuse the stack space (which it often does) 3. size() is called and returns a reference to a Display field. Once Display goes out of scope (and destroyed), the reference saved in |size| points to an already potentially reused memory, and not to what it meant to refer to. 4. |size| is then used and AddressSanitizer with use-after-scope check correctly points out to the issue. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 Review-Url: https://codereview.chromium.org/2643943003 Cr-Commit-Position: refs/heads/master@{#444812} [modify] https://crrev.com/d377464c5d3b7aaa13823000932c1a23f3c4d01c/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
,
Jan 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/191d9a603c5b154dcf3ba8efeeb632cd80c4e411 commit 191d9a603c5b154dcf3ba8efeeb632cd80c4e411 Author: krasin <krasin@chromium.org> Date: Thu Jan 19 19:58:31 2017 ash: fix multiple stack-use-after-scope issues with GetPrimaryDisplay use. The common pattern is to save a reference to the display work area into a local variable, but the reference is to the stack allocated Display object that goes out of scope immediately after taking the reference. The remedy is to have |work_area| as a proper variable. For example: const gfx::Rect& work_area = display::Screen::GetScreen()->GetPrimaryDisplay().work_area(); becomes const gfx::Rect work_area = display::Screen::GetScreen()->GetPrimaryDisplay().work_area(); The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 Review-Url: https://codereview.chromium.org/2643853003 Cr-Commit-Position: refs/heads/master@{#444823} [modify] https://crrev.com/191d9a603c5b154dcf3ba8efeeb632cd80c4e411/ash/content/keyboard_overlay/keyboard_overlay_delegate.cc [modify] https://crrev.com/191d9a603c5b154dcf3ba8efeeb632cd80c4e411/ash/frame/caption_buttons/frame_size_button_unittest.cc [modify] https://crrev.com/191d9a603c5b154dcf3ba8efeeb632cd80c4e411/ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc
,
Jan 19 2017
browser_tests failures are not reproducible on my machine: possibly one of the already applied fixes is responsible for that. I have sent another set of try jobs to https://codereview.chromium.org/2451973004/ and we'll see how it looks tomorrow.
,
Jan 20 2017
So, linux and ChromeOS are green; Mac has multiple tests failing (see #23), but all of them are due to a single issue. See the stack trace below:
==24329==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fff54da2ca0 at pc 0x00011b1c431b bp 0x7fff54da2c70 sp 0x7fff54da2c68
WRITE of size 8 at 0x7fff54da2ca0 thread T0
#0 0x11b1c431a in HistoryMenuBridge::ClearMenuSection(NSMenu*, long) (in interactive_ui_tests) + 1882
#1 0x11b1c2914 in HistoryMenuBridge::TabRestoreServiceChanged(sessions::TabRestoreService*) (in interactive_ui_tests) + 308
#2 0x11b1c182b in HistoryMenuBridge::HistoryMenuBridge(Profile*) (in interactive_ui_tests) + 1515
#3 0x11090f6e3 in -[AppController windowChangedToProfile:] (in interactive_ui_tests) + 1523
#4 0x7fff8ff96e0b in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ (in CoreFoundation) + 11
#5 0x7fff8fe8a82c in _CFXNotificationPost (in CoreFoundation) + 2892
#6 0x7fff96eacdd9 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 67
#7 0x7fff928c6a46 in _NXShowKeyAndMain (in AppKit) + 170
#8 0x7fff927639c5 in -[NSApplication sendEvent:] (in AppKit) + 7126
#9 0x110a23482 in __34-[BrowserCrApplication sendEvent:]_block_invoke (in interactive_ui_tests) + 578
#10 0x113d39b59 in base::mac::CallWithEHFrame(void () block_pointer) (in interactive_ui_tests) + 9
#11 0x110a22c45 in -[BrowserCrApplication sendEvent:] (in interactive_ui_tests) + 1029
#12 0x7fff925b29f8 in -[NSApplication run] (in AppKit) + 645
#13 0x113d9319f in base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) (in interactive_ui_tests) + 1103
#14 0x113d90e7c in base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) (in interactive_ui_tests) + 412
#15 0x113d7e2d0 in base::MessageLoop::RunHandler() (in interactive_ui_tests) + 1280
#16 0x113e1c090 in base::RunLoop::Run() (in interactive_ui_tests) + 496
#17 0x113a57d51 in -[WindowedNSNotificationObserver waitForCount:] (in interactive_ui_tests) + 977
#18 0x10b28fc9f in ui_test_utils::ShowAndFocusNativeWindow(NSWindow*) (in interactive_ui_tests) + 623
#19 0x10b28dca1 in ui_test_utils::BringBrowserWindowToFront(Browser const*) (in interactive_ui_tests) + 257
#20 0x10b1fcb60 in OmniboxViewTest::SetUpOnMainThread() (in interactive_ui_tests) + 288
#21 0x1108dc4c9 in InProcessBrowserTest::RunTestOnMainThreadLoop() (in interactive_ui_tests) + 969
#22 0x1120f9d1f in content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() (in interactive_ui_tests) + 1071
#23 0x110a3197e in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() (in interactive_ui_tests) + 11182
#24 0x110a2e985 in ChromeBrowserMainParts::PreMainMessageLoopRun() (in interactive_ui_tests) + 277
#25 0x10e64d7bc in content::BrowserMainLoop::PreMainMessageLoopRun() (in interactive_ui_tests) + 364
#26 0x10f5209ea in content::StartupTaskRunner::RunAllTasksNow() (in interactive_ui_tests) + 634
#27 0x10e647274 in content::BrowserMainLoop::CreateStartupTasks() (in interactive_ui_tests) + 1540
#28 0x10e658a02 in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) (in interactive_ui_tests) + 1506
#29 0x10e63ede1 in content::BrowserMain(content::MainFunctionParams const&) (in interactive_ui_tests) + 353
#30 0x11086cd1e in content::RunNamedProcessTypeMain(std::__1::basic_string\u003Cchar, std::__1::char_traits\u003Cchar>, std::__1::allocator\u003Cchar> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) (in interactive_ui_tests) + 1406
#31 0x11086ef22 in content::ContentMainRunnerImpl::Run() (in interactive_ui_tests) + 1506
#32 0x11086c05d in content::ContentMain(content::ContentMainParams const&) (in interactive_ui_tests) + 141
#33 0x1120f8db3 in content::BrowserTestBase::SetUp() (in interactive_ui_tests) + 5411
#34 0x1108d949c in InProcessBrowserTest::SetUp() (in interactive_ui_tests) + 1404
#35 0x1139e8bee in testing::Test::Run() (in interactive_ui_tests) + 462
#36 0x1139eb0e3 in testing::TestInfo::Run() (in interactive_ui_tests) + 1363
#37 0x1139ec676 in testing::TestCase::Run() (in interactive_ui_tests) + 1318
#38 0x1139ffd96 in testing::internal::UnitTestImpl::RunAllTests() (in interactive_ui_tests) + 2566
#39 0x1139ff2dc in testing::UnitTest::Run() (in interactive_ui_tests) + 412
#40 0x113c7e977 in base::TestSuite::Run() (in interactive_ui_tests) + 519
#41 0x10b2902d8 in InteractiveUITestSuiteRunner::RunTestSuite(int, char**) (in interactive_ui_tests) + 264
#42 0x11210c543 in content::LaunchTests(content::TestLauncherDelegate*, int, int, char**) (in interactive_ui_tests) + 1267
#43 0x10b290110 in main (in interactive_ui_tests) + 256
#44 0x7fff8c6e75fc in start (in libdyld.dylib) + 0
Address 0x7fff54da2ca0 is located in stack of thread T0 at offset 32 in frame
#0 0x11b1c3bcf in HistoryMenuBridge::ClearMenuSection(NSMenu*, long) (in interactive_ui_tests) + 15
This frame has 3 object(s):
[32, 40) 'menu_item:256' \u003C== Memory access at offset 32 is inside this variable
[64, 128) 'state.ptr'
[160, 288) 'items.ptr'
,
Jan 20 2017
The CL to enable it on non-Mac is out: https://codereview.chromium.org/2451973004/ I will deal with the Mac issue later, in particular, because it will require to learn a bit of Objective C and / or find an expert around.
,
Jan 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/866204443431a62c1b6e1404ed3c19c3f529ce45 commit 866204443431a62c1b6e1404ed3c19c3f529ce45 Author: krasin <krasin@chromium.org> Date: Fri Jan 20 19:00:26 2017 Enable use-after-scope check in ASAN configs. At this point, everything but Mac has been fixed. On Mac, there's an outstanding issue, which will be fixed later. BUG= 649897 Review-Url: https://codereview.chromium.org/2451973004 Cr-Commit-Position: refs/heads/master@{#445114} [modify] https://crrev.com/866204443431a62c1b6e1404ed3c19c3f529ce45/build/config/sanitizers/BUILD.gn
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73ce07497d3279402d2f51a03e55be8c4612ff50 commit 73ce07497d3279402d2f51a03e55be8c4612ff50 Author: haraken <haraken@chromium.org> Date: Mon Jan 23 07:41:09 2017 Revert of Enable use-after-scope check in ASAN configs. (patchset #5 id:80001 of https://codereview.chromium.org/2451973004/ ) Reason for revert: This CL caused crashes in Linux ASAN. https://bugs.chromium.org/p/chromium/issues/detail?id=683459 Original issue's description: > Enable use-after-scope check in ASAN configs. > > At this point, everything but Mac has been fixed. On Mac, there's an outstanding issue, which will be fixed later. > > BUG= 649897 > > Review-Url: https://codereview.chromium.org/2451973004 > Cr-Commit-Position: refs/heads/master@{#445114} > Committed: https://chromium.googlesource.com/chromium/src/+/866204443431a62c1b6e1404ed3c19c3f529ce45 TBR=thakis@chromium.org,hans@chromium.org,achuith@chromium.org,krasin@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 649897 Review-Url: https://codereview.chromium.org/2645263003 Cr-Commit-Position: refs/heads/master@{#445331} [modify] https://crrev.com/73ce07497d3279402d2f51a03e55be8c4612ff50/build/config/sanitizers/BUILD.gn
,
Jan 23 2017
Thank you for reverting. To summarize, the change has broken a number of configurations: 1. Chrome OS uses an old Clang toolchain that does not support -fsanitize-use-after-scope: https://crbug.com/683445 I will exclude Chrome OS when I reland the CL. No other remedy comes to my mind. 2. Some Windows bots were broken, because clang-cl does not expose the flag: https://crbug.com/683966 Windows will be excluded when I reland the CL, but we can probably fix this on the Clang side. 3. One test case in webkit_unit_tests had an unfixed use-after-scope issue on Linux: https://crbug.com/683459 I will fix it before relanding the CL. I am surprised that this was not caught by any of the trybots.
,
Jan 23 2017
Correction, it's webkit_tests, not webkit_unit_tests failing.
,
Jan 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6de90795ea5ee5c080e3266dceedd98f66b045e commit a6de90795ea5ee5c080e3266dceedd98f66b045e Author: krasin <krasin@chromium.org> Date: Tue Jan 24 00:25:37 2017 blink: fix use-after-scope issues in CSSInterpolationType. The problem was in the following line: const AtomicString& propertyName = getProperty().customPropertyName(); <consequent use of propertyName> getProperty() returns PropertyHandle value that is allocated on the stack as a temporary variable with the scope of one statement. Then customPropertyName() returns a reference to this temp variable. Once the statement is over, PropertyHandle value goes out of scope and destroyed. By the time propertyName is used, it already points to a potentially reused stack address. And even if it's not yet reused, the object is destroyed, so the value is invalid anyway. The fix is to save ProperyHandle value in a local variable with the large enough scope to cover all propertyName uses. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 , 683459 Review-Url: https://codereview.chromium.org/2649903005 Cr-Commit-Position: refs/heads/master@{#445559} [modify] https://crrev.com/a6de90795ea5ee5c080e3266dceedd98f66b045e/third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp
,
Jan 24 2017
I have relanded the check in https://codereview.chromium.org/2654623002/
,
Jan 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/269942eb79516fad019284211999bc92d382c138 commit 269942eb79516fad019284211999bc92d382c138 Author: Alan Cutter <alancutter@chromium.org> Date: Wed Jan 25 01:52:46 2017 blink: fix use-after-scope issues in CSSInterpolationType. The problem was in the following line: const AtomicString& propertyName = getProperty().customPropertyName(); <consequent use of propertyName> getProperty() returns PropertyHandle value that is allocated on the stack as a temporary variable with the scope of one statement. Then customPropertyName() returns a reference to this temp variable. Once the statement is over, PropertyHandle value goes out of scope and destroyed. By the time propertyName is used, it already points to a potentially reused stack address. And even if it's not yet reused, the object is destroyed, so the value is invalid anyway. The fix is to save ProperyHandle value in a local variable with the large enough scope to cover all propertyName uses. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 , 683459 , 683493 Review-Url: https://codereview.chromium.org/2649903005 Cr-Commit-Position: refs/heads/master@{#445559} (cherry picked from commit a6de90795ea5ee5c080e3266dceedd98f66b045e) Review-Url: https://codereview.chromium.org/2655663005 . Cr-Commit-Position: refs/branch-heads/2987@{#79} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/269942eb79516fad019284211999bc92d382c138/third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp
,
Jan 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/539f64c0e54adc91ae1276948771a3538cf4261b commit 539f64c0e54adc91ae1276948771a3538cf4261b Author: krasin <krasin@chromium.org> Date: Fri Jan 27 00:42:02 2017 Enable use-after-scope check on Chrome OS. Now that Chrome OS uses a recent enough Clang toolchain, it's possible to enable the check there too. BUG= 683445 , 649897 Review-Url: https://codereview.chromium.org/2654383002 Cr-Commit-Position: refs/heads/master@{#446503} [modify] https://crrev.com/539f64c0e54adc91ae1276948771a3538cf4261b/build/config/sanitizers/BUILD.gn
,
Feb 28 2017
what's the current status?
,
Feb 28 2017
It's deployed on Linux and Chrome OS.
Not on Mac, not on Windows and not on Android.
I am in favor to declare the issue as fixed, as most of the paths are already covered by ASAN {try,build}-bots. Eventually, we might want to expand into more OSes.
|
|||
►
Sign in to add a comment |
|||
Comment 1 by krasin@chromium.org
, Oct 27 2016