From 767943850f04700655ca26c34b9366dd0b40c1e4 Mon Sep 17 00:00:00 2001 From: Martin Kustermann Date: Tue, 25 Feb 2020 10:32:01 +0000 Subject: [PATCH] [vm] Enable check_initialization_order for ASAN, ensure tools/test.py always passes ASAN options, make Random be initialized during VM startup Running tools/test.py locally behaves differently than on the bots because the bots will have special environment variables set. => We want also local runs of tools/test.py to report ASAN errors, so this CL moves the setting of environment variables to tools/test.py Furthermore we enable "additional check_initialization_order=true" asan option to detect ordering issues when initializing global state. Make the IsolateGroup::isolate_group_random_ heap allocated on VM startup to avoid initialization ordering issue (turns out our `Random` can depend on embedder entropy source callback being available) Also add locking around assigning of isolate group ids -- without it we could have two threads racing and possibly getting the same id. See b/149978682 Change-Id: I41b9a34c66934dcc0d3804233f5acf09004ba3bd Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/136630 Commit-Queue: Martin Kustermann Reviewed-by: Ryan Macnak Reviewed-by: Alexander Thomas Reviewed-by: Alexander Aprelev --- pkg/test_runner/lib/src/options.dart | 21 ++++++++ pkg/test_runner/lib/src/test_case.dart | 5 +- runtime/vm/dart.cc | 2 +- runtime/vm/isolate.cc | 9 +++- runtime/vm/isolate.h | 4 +- tools/bots/test_matrix.json | 72 ++++++++------------------ tools/build.py | 36 ++++++++++--- 7 files changed, 87 insertions(+), 62 deletions(-) diff --git a/pkg/test_runner/lib/src/options.dart b/pkg/test_runner/lib/src/options.dart index 95f92173696..5de5a7d2d53 100644 --- a/pkg/test_runner/lib/src/options.dart +++ b/pkg/test_runner/lib/src/options.dart @@ -7,6 +7,7 @@ import 'dart:io'; import 'package:smith/smith.dart'; import 'package:test_runner/src/test_configurations.dart'; +import 'package:path/path.dart' as path; import 'configuration.dart'; import 'path.dart'; @@ -993,3 +994,23 @@ class OptionParseException implements Exception { void _fail(String message) { throw OptionParseException(message); } + +// Returns a map of environment variables to be used with sanitizers. +final Map sanitizerEnvironmentVariables = (() { + final environment = {}; + final testMatrixFile = "tools/bots/test_matrix.json"; + final config = json.decode(File(testMatrixFile).readAsStringSync()); + config['sanitizer_options'].forEach((String key, dynamic value) { + environment[key] = value as String; + }); + var symbolizerPath = + config['sanitizer_symbolizer'][Platform.operatingSystem] as String; + if (symbolizerPath != null) { + symbolizerPath = path.join(Directory.current.path, symbolizerPath); + environment['ASAN_SYMBOLIZER_PATH'] = symbolizerPath; + environment['MSAN_SYMBOLIZER_PATH'] = symbolizerPath; + environment['TSAN_SYMBOLIZER_PATH'] = symbolizerPath; + } + + return environment; +})(); diff --git a/pkg/test_runner/lib/src/test_case.dart b/pkg/test_runner/lib/src/test_case.dart index c5e278abc70..264d73dcdfc 100644 --- a/pkg/test_runner/lib/src/test_case.dart +++ b/pkg/test_runner/lib/src/test_case.dart @@ -12,6 +12,7 @@ import "package:status_file/expectation.dart"; import 'command.dart'; import 'command_output.dart'; import 'configuration.dart'; +import 'options.dart'; import 'output_log.dart'; import 'process_queue.dart'; import 'test_file.dart'; @@ -381,8 +382,8 @@ class RunningProcess { } Map _createProcessEnvironment() { - var environment = Map.from(io.Platform.environment); - + final environment = Map.from(io.Platform.environment); + environment.addAll(sanitizerEnvironmentVariables); if (command.environmentOverrides != null) { for (var key in command.environmentOverrides.keys) { environment[key] = command.environmentOverrides[key]; diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc index c3011ea529f..4f2c567f4c5 100644 --- a/runtime/vm/dart.cc +++ b/runtime/vm/dart.cc @@ -208,8 +208,8 @@ char* Dart::Init(const uint8_t* vm_isolate_snapshot, Timeline::Init(); TimelineBeginEndScope tbes(Timeline::GetVMStream(), "Dart::Init"); #endif - Isolate::InitVM(); IsolateGroup::Init(); + Isolate::InitVM(); PortMap::Init(); FreeListElement::Init(); ForwardingCorpse::Init(); diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 03e1c7fae05..b75ee199979 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -235,6 +235,10 @@ IsolateGroup::IsolateGroup(std::shared_ptr source, shared_class_table_(new SharedClassTable()), store_buffer_(new StoreBuffer()), heap_(nullptr) { + { + WriteRwLocker wl(ThreadState::Current(), isolate_groups_rwlock_); + id_ = isolate_group_random_->NextUInt64(); + } } IsolateGroup::~IsolateGroup() { @@ -566,9 +570,12 @@ void IsolateGroup::Init() { isolate_groups_rwlock_ = new RwLock(); ASSERT(isolate_groups_ == nullptr); isolate_groups_ = new IntrusiveDList(); + isolate_group_random_ = new Random(); } void IsolateGroup::Cleanup() { + delete isolate_group_random_; + isolate_group_random_ = nullptr; delete isolate_groups_rwlock_; isolate_groups_rwlock_ = nullptr; ASSERT(isolate_groups_->IsEmpty()); @@ -2365,7 +2372,7 @@ Dart_IsolateShutdownCallback Isolate::shutdown_callback_ = nullptr; Dart_IsolateCleanupCallback Isolate::cleanup_callback_ = nullptr; Dart_IsolateGroupCleanupCallback Isolate::cleanup_group_callback_ = nullptr; -Random IsolateGroup::isolate_group_random_ = {}; +Random* IsolateGroup::isolate_group_random_ = nullptr; Monitor* Isolate::isolate_creation_monitor_ = nullptr; intptr_t Isolate::application_isolates_count_ = 0; intptr_t Isolate::total_isolates_count_ = 0; diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index 65c9bf80727..99381a83838 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -532,9 +532,9 @@ class IsolateGroup : public IntrusiveDListEntry { static RwLock* isolate_groups_rwlock_; static IntrusiveDList* isolate_groups_; + static Random* isolate_group_random_; - static Random isolate_group_random_; - uint64_t id_ = isolate_group_random_.NextUInt64(); + uint64_t id_ = 0; std::unique_ptr shared_class_table_; std::unique_ptr store_buffer_; diff --git a/tools/bots/test_matrix.json b/tools/bots/test_matrix.json index 00cea13bde1..35c5b74da60 100644 --- a/tools/bots/test_matrix.json +++ b/tools/bots/test_matrix.json @@ -1378,21 +1378,13 @@ "arguments": [ "--sanitizer=asan", "runtime" - ], - "environment": { - "ASAN_OPTIONS": "handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", - "ASAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] }, { "name": "vm tests", "arguments": [ "-ndartk-asan-linux-release-${arch}" - ], - "environment": { - "ASAN_OPTIONS": "handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", - "ASAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] } ] }, @@ -1420,21 +1412,13 @@ "arguments": [ "--sanitizer=lsan", "runtime" - ], - "environment": { - "ASAN_OPTIONS": "handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", - "ASAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] }, { "name": "vm tests", "arguments": [ "-ndartk-lsan-linux-release-${arch}" - ], - "environment": { - "ASAN_OPTIONS": "handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", - "ASAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] } ] }, @@ -1461,21 +1445,13 @@ "arguments": [ "--sanitizer=msan", "runtime" - ], - "environment": { - "MSAN_OPTIONS": "handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", - "MSAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] }, { "name": "vm tests", "arguments": [ "-ndartk-msan-linux-release-${arch}" - ], - "environment": { - "MSAN_OPTIONS": "handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", - "MSAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] } ] }, @@ -1502,21 +1478,13 @@ "arguments": [ "--sanitizer=tsan", "runtime" - ], - "environment": { - "TSAN_OPTIONS": "handle_segv=0:disable_coredump=0:abort_on_error=1", - "TSAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] }, { "name": "vm tests", "arguments": [ "-ndartk-tsan-linux-release-${arch}" - ], - "environment": { - "TSAN_OPTIONS": "handle_segv=0:disable_coredump=0:abort_on_error=1", - "TSAN_SYMBOLIZER_PATH": "buildtools/linux-x64/clang/bin/llvm-symbolizer" - } + ] } ] }, @@ -1544,19 +1512,13 @@ "arguments": [ "--sanitizer=ubsan", "runtime" - ], - "environment": { - "UBSAN_OPTIONS": "handle_segv=0:disable_coredump=0:abort_on_error=1" - } + ] }, { "name": "vm tests", "arguments": [ "-ndartk-ubsan-linux-release-${arch}" - ], - "environment": { - "UBSAN_OPTIONS": "handle_segv=0:disable_coredump=0:abort_on_error=1" - } + ] } ] }, @@ -3496,5 +3458,15 @@ } ] } - ] -} \ No newline at end of file + ], + "sanitizer_options": { + "ASAN_OPTIONS": "check_initialization_order=true:handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", + "MSAN_OPTIONS": "check_initialization_order=true:handle_segv=0:detect_leaks=1:detect_stack_use_after_return=0:disable_coredump=0:abort_on_error=1", + "TSAN_OPTIONS": "handle_segv=0:disable_coredump=0:abort_on_error=1", + "UBSAN_OPTIONS": "handle_segv=0:disable_coredump=0:abort_on_error=1" + }, + "sanitizer_symbolizer": { + "linux": "buildtools/linux-x64/clang/bin/llvm-symbolizer", + "macos": "buildtools/mac-x64/clang/bin/llvm-symbolizer" + } +} diff --git a/tools/build.py b/tools/build.py index 17ee88d3753..20ac683988b 100755 --- a/tools/build.py +++ b/tools/build.py @@ -4,6 +4,8 @@ # for details. All rights reserved. Use of this source code is governed by a # BSD-style license that can be found in the LICENSE file. +import io +import json import multiprocessing import optparse import os @@ -312,10 +314,10 @@ def BuildOneConfig(options, targets, target_os, mode, arch, sanitizer): return (build_config, command, using_goma) -def RunOneBuildCommand(build_config, args): +def RunOneBuildCommand(build_config, args, env): start_time = time.time() print(' '.join(args)) - process = subprocess.Popen(args, stdin=None) + process = subprocess.Popen(args, env=env, stdin=None) process.wait() if process.returncode != 0: NotifyBuildDone(build_config, success=False, start=start_time) @@ -326,10 +328,11 @@ def RunOneBuildCommand(build_config, args): return 0 -def RunOneGomaBuildCommand(args): +def RunOneGomaBuildCommand(options): + (env, args) = options try: print(' '.join(args)) - process = subprocess.Popen(args, stdin=None) + process = subprocess.Popen(args, env=env, stdin=None) process.wait() print(' '.join(args) + " done.") return process.returncode @@ -337,6 +340,21 @@ def RunOneGomaBuildCommand(args): return 1 +def SanitizerEnvironmentVariables(): + with io.open('tools/bots/test_matrix.json', encoding='utf-8') as fd: + config = json.loads(fd.read()) + env = dict() + for k, v in config['sanitizer_options'].items(): + env[str(k)] = str(v) + symbolizer_path = config['sanitizer_symbolizer'].get(HOST_OS, None) + if symbolizer_path: + symbolizer_path = str(os.path.join(DART_ROOT, symbolizer_path)) + env['ASAN_SYMBOLIZER_PATH'] = symbolizer_path + env['MSAN_SYMBOLIZER_PATH'] = symbolizer_path + env['TSAN_SYMBOLIZER_PATH'] = symbolizer_path + return env + + def Main(): starttime = time.time() # Parse the options. @@ -354,6 +372,12 @@ def Main(): if not GenerateBuildfilesIfNeeded(): return 1 + # If binaries are built with sanitizers we should use those flags. + # If the binaries are not built with sanitizers the flag should have no + # effect. + env = dict(os.environ) + env.update(SanitizerEnvironmentVariables()) + # Build all targets for each requested configuration. configs = [] for target_os in options.os: @@ -370,8 +394,8 @@ def Main(): if args is None: return 1 if goma: - goma_builds.append(args) - elif RunOneBuildCommand(build_config, args) != 0: + goma_builds.append([env, args]) + elif RunOneBuildCommand(build_config, args, env=env) != 0: return 1 # Run goma builds in parallel.