Status: Unconfirmed Owner: ---- Labels: OS-Linux Area-Misc Size-Medium Type-Bug
New issue 18514 by timurrrr: Data race in SafeBrowsingService http://code.google.com/p/chromium/issues/detail?id=18514 Chrome Version : r22000 OS + version : Linux, possibly others as well The data race was detected using ThreadSanitizer (see http://code.google.com/p/data-race-test/wiki/ThreadSanitizer ) The race can be proved using RaceChecker class (see http://code.google.com/p/data-race-test/wiki/RaceCheckerClass ) What steps will reproduce the problem? 1. Apply the attached patch * adds RaceChecker class files into base/..., * adds dependencies for these files * puts RaceChecker calls and extra logging into SafeBrowsingService 2. gclient sync && hammer ui_tests 3. RACECHECKER=2 RACECHECKER_SLEEP_MS=5000 ./sconsbuild/Debug/ui_tests --gtest_filter="ViewSourceTest.DoesBrowserRenderInViewSource" Please take a look at those fprintf lines I've added to chrome/browser/safe_browsing/safe_browsing_service.cc before looking at the log below. What happens instead? [ RUN ] ViewSourceTest.DoesBrowserRenderInViewSource [15589:15589:0805/190639:455187445872:ERROR:src/chrome/test/ui/ui_test.cc(470)] Not implemented reached in void UITest::LaunchBrowser(const CommandLine&, bool) [15593:15593:455187549881:ERROR:src/chrome/browser/google_update_settings_linux.cc(60)] Not implemented reached in static bool GoogleUpdateSettings::GetLanguage(std::wstring*) DATABASE LOADED, NOT YET PUBLISHED RACECHECKER: write at 0xb97d594 [15589:15589:0805/190640:455188047443:INFO:src/net/socket/ssl_test_util.cc(225)] Trying to launch python ... [15589:15589:0805/190644:455192051740:INFO:src/net/socket/ssl_test_util.cc(241)] Started on port 1337 Errr, where's DATABASE? RACECHECKER: read at 0xb97d594 Race found between these points === writer: src/sconsbuild/Debug/chrome(_ZN19SafeBrowsingService11GetDatabaseEv+0x322)[0x94b5b9a] src/sconsbuild/Debug/chrome(_ZN19SafeBrowsingService14OnDBInitializeEv+0xbb)[0x94b6425] src/sconsbuild/Debug/chrome(_Z16DispatchToMethodI19SafeBrowsingServiceMS0_FvvEEvPT_T0_RK6Tuple0+0x3a)[0x94bae54] src/sconsbuild/Debug/chrome(_ZN14RunnableMethodI19SafeBrowsingServiceMS0_FvvE6Tuple0E3RunEv+0x39)[0x94bae8f] src/sconsbuild/Debug/chrome(_ZN11MessageLoop7RunTaskEP4Task+0xce)[0x984ddda] ... === reader: src/sconsbuild/Debug/chrome(_ZN19SafeBrowsingService8CheckUrlERK4GURLPNS_6ClientE+0x104)[0x94b6f6a] src/sconsbuild/Debug/chrome(_ZN27SafeBrowsingResourceHandlerC1EP15ResourceHandleriiRK4GURLN12ResourceType4TypeEP19SafeBrowsingServiceP22ResourceDispatcherHostPNS9_8ReceiverE+0x127)[0x94ac4c9] src/sconsbuild/Debug/chrome(_ZN22ResourceDispatcherHost12BeginRequestEiRK28ViewHostMsg_Resource_RequestPN3IPC7MessageEi+0x85c)[0x94a2e72] src/sconsbuild/Debug/chrome(_ZN22ResourceDispatcherHost17OnRequestResourceERKN3IPC7MessageEiRK28ViewHostMsg_Resource_Request+0x36)[0x94a305c] src/sconsbuild/Debug/chrome(_ZN3IPC16MessageWithTupleI6Tuple2Ii28ViewHostMsg_Resource_RequestEE8DispatchI22ResourceDispatcherHostiRKS2_EEbPKNS_7MessageEPT_MSC_FvRSA_T0_T1_E+0x8d)[0x94ab0f5] src/sconsbuild/Debug/chrome(_ZN22ResourceDispatcherHost17OnMessageReceivedERKN3IPC7MessageEPNS_8ReceiverEPb+0xea)[0x94a0c44] src/sconsbuild/Debug/chrome(_ZN21ResourceMessageFilter17OnMessageReceivedERKN3IPC7MessageE+0x3c)[0x96b8a60] src/sconsbuild/Debug/chrome(_ZN3IPC12ChannelProxy7Context10TryFiltersERKNS_7MessageE+0x6a)[0x9d2d3ae] src/sconsbuild/Debug/chrome(_ZN3IPC11SyncChannel11SyncContext17OnMessageReceivedERKNS_7MessageE+0x18)[0x9d3476c] ... ============================ As you can see, in SafeBrowsingService::GetDatabase() the publishing of a database 397 398 database_ = database; 399 fprintf(stderr, "DATABASE PUBLISHED\n"); happens _after_ notifying other threads about it availability: 381 SafeBrowsingDatabase* database = SafeBrowsingDatabase::Create(); 382 Callback0::Type* chunk_callback = 383 NewCallback(this, &SafeBrowsingService::ChunkInserted); 384 bool init_success = database->Init(path, chunk_callback); 385 fprintf(stderr, "DATABASE LOADED, NOT YET PUBLISHED\n"); 386 387 // This creates an implicit memory 388 io_loop_->PostTask(FROM_HERE, NewRunnableMethod( 389 this, &SafeBrowsingService::DatabaseLoadComplete, !init_success)); No other means of synchronization (Mutex/Event) are involved. As a result, SafeBrowsingService::CheckUrl() may return "true" (that means "URL is fine"). CheckUrl() can also be called from SafeBrowsingService::RunQueuedClients(). Seems like more blocks like "if (!database_) {...}" may mistakenly be executed as well. I'm not very familiar with the source code but I guess having a data race in a class related to safe browsing could be harmful even if it doesn't lead to crashes. Attachments: SafeBrowsing_database_race.patch 12.4 KB -- You received this message because you are listed in the owner or CC fields of this issue, or because you starred this issue. You may adjust your issue notification preferences at: http://code.google.com/hosting/settings --~--~---------~--~----~------------~-------~--~----~ Automated mail from issue updates at http://crbug.com/ Subscription options: http://groups.google.com/group/chromium-bugs -~----------~----~----~----~------~----~------~--~---
