diff --git a/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc b/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc index c6fde24644f..9686fae4189 100644 --- a/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc +++ b/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc @@ -1275,4 +1275,33 @@ DART_EXPORT void SetFfiNativeResolverForTest(Dart_Handle url) { ENSURE(!Dart_IsError(result)); } +//////////////////////////////////////////////////////////////////////////////// +// Helper for the regression test for b/216834909 +//////////////////////////////////////////////////////////////////////////////// + +#if defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_ANDROID) || \ + defined(DART_HOST_OS_MACOS) +static bool Regress216834909_hang_at_exit = true; + +static void Regress216834909_AtExit() { + if (Regress216834909_hang_at_exit) { + while (true) { + sleep(60 * 60); // Sleep for 1 hour. + } + } +} + +DART_EXPORT void Regress216834909_SetAtExit(int64_t install) { + if (install != 0) { + // Set and arm atexit routine. + atexit(&Regress216834909_AtExit); + Regress216834909_hang_at_exit = true; + } else { + // Disarm atexit routine. + Regress216834909_hang_at_exit = false; + } +} +#endif // defined(DART_HOST_OS_LINUX) || defined(DART_HOST_OS_ANDROID) || \ + // defined(DART_HOST_OS_MACOS) + } // namespace dart diff --git a/runtime/bin/process_android.cc b/runtime/bin/process_android.cc index 16955e947c4..955efdc905c 100644 --- a/runtime/bin/process_android.cc +++ b/runtime/bin/process_android.cc @@ -435,7 +435,7 @@ class ProcessStarter { int bytes_read = FDUtils::ReadFromBlocking(read_in_[0], &msg, sizeof(msg)); if (bytes_read != sizeof(msg)) { perror("Failed receiving notification message"); - exit(1); + _exit(1); } if (Process::ModeIsAttached(mode_)) { ExecProcess(); @@ -568,13 +568,15 @@ class ProcessStarter { execvp(realpath, const_cast(program_arguments_)); ReportChildError(); } else { - // Exit the intermediate process. - exit(0); + // Exit the intermediate process. Avoid calling any atexit callbacks + // to avoid potential issues (e.g. deadlocks). + _exit(0); } } } else { - // Exit the intermediate process. - exit(0); + // Exit the intermediate process. Avoid calling any atexit callbacks + // to avoid potential issues (e.g. deadlocks). + _exit(0); } } diff --git a/runtime/bin/process_linux.cc b/runtime/bin/process_linux.cc index 95a9c86d898..ae7f1b59c86 100644 --- a/runtime/bin/process_linux.cc +++ b/runtime/bin/process_linux.cc @@ -170,8 +170,7 @@ class ExitCodeHandler { // Wake up the [ExitCodeHandler] thread which is blocked on `wait()` (see // [ExitCodeHandlerEntry]). if (TEMP_FAILURE_RETRY(fork()) == 0) { - // We avoid running through registered atexit() handlers because that is - // unnecessary work. + // Avoid calling any atexit callbacks to prevent deadlocks. _exit(0); } @@ -437,7 +436,7 @@ class ProcessStarter { int bytes_read = FDUtils::ReadFromBlocking(read_in_[0], &msg, sizeof(msg)); if (bytes_read != sizeof(msg)) { perror("Failed receiving notification message"); - exit(1); + _exit(1); } if (Process::ModeIsAttached(mode_)) { ExecProcess(); @@ -569,13 +568,15 @@ class ProcessStarter { execvp(realpath, const_cast(program_arguments_)); ReportChildError(); } else { - // Exit the intermediate process. - exit(0); + // Exit the intermediate process. Avoid calling any atexit callbacks + // to avoid potential issues (e.g. deadlocks). + _exit(0); } } } else { - // Exit the intermediate process. - exit(0); + // Exit the intermediate process. Avoid calling any atexit callbacks + // to avoid potential issues (e.g. deadlocks). + _exit(0); } } @@ -728,7 +729,8 @@ class ProcessStarter { close(exec_control_[1]); // We avoid running through registered atexit() handlers because that is - // unnecessary work. + // unnecessary work. It can also cause deadlocks on exit in the forked + // process. _exit(1); } diff --git a/runtime/bin/process_macos.cc b/runtime/bin/process_macos.cc index 6bbbfae2ac3..f174679d046 100644 --- a/runtime/bin/process_macos.cc +++ b/runtime/bin/process_macos.cc @@ -168,7 +168,7 @@ class ExitCodeHandler { // Fork to wake up waitpid. if (TEMP_FAILURE_RETRY(fork()) == 0) { - exit(0); + _Exit(0); } monitor_->Notify(); @@ -437,7 +437,7 @@ class ProcessStarter { int bytes_read = FDUtils::ReadFromBlocking(read_in_[0], &msg, sizeof(msg)); if (bytes_read != sizeof(msg)) { perror("Failed receiving notification message"); - exit(1); + _Exit(1); } if (Process::ModeIsAttached(mode_)) { ExecProcess(); @@ -535,13 +535,15 @@ class ProcessStarter { execvp(path_, const_cast(program_arguments_)); ReportChildError(); } else { - // Exit the intermeiate process. - exit(0); + // Exit the intermeiate process. Avoid any atexit callbacks + // to prevent deadlocks. + _Exit(0); } } } else { - // Exit the intermeiate process. - exit(0); + // Exit the intermeiate process. Avoid any atexit callbacks + // to prevent deadlocks. + _Exit(0); } } @@ -695,7 +697,8 @@ class ProcessStarter { strlen(os_error_message) + 1); } close(exec_control_[1]); - exit(1); + // Avoid calling any atexit callbacks to prevent deadlocks. + _Exit(1); } void ReportPid(int pid) { diff --git a/runtime/tests/vm/dart/regress_b_216834909_test.dart b/runtime/tests/vm/dart/regress_b_216834909_test.dart new file mode 100644 index 00000000000..a73ff2d855f --- /dev/null +++ b/runtime/tests/vm/dart/regress_b_216834909_test.dart @@ -0,0 +1,34 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// 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. +// +// SharedObjects=ffi_test_functions + +// Regression test for b/216834909. +// +// Check that subprocess spawning implementation uses _exit rather than exit on +// paths which terminate fork child without exec-ing. + +import 'dart:async'; +import 'dart:ffi'; +import 'dart:io'; +import 'dart:isolate'; + +import "package:expect/expect.dart"; +import '../../../../tests/ffi/dylib_utils.dart'; + +final ffiTestFunctions = dlopenPlatformSpecific('ffi_test_functions'); + +final setAtExit = + ffiTestFunctions.lookupFunction( + 'Regress216834909_SetAtExit'); + +main(List args) async { + // We only care about platforms which use fork/exec. + if (!Platform.isLinux && !Platform.isAndroid && !Platform.isMacOS) { + return; + } + setAtExit(1); // Install at exit handler. + await Process.start('true', [], mode: ProcessStartMode.detached); + setAtExit(0); // Clear at exit handler. +} diff --git a/runtime/tests/vm/dart_2/regress_b_216834909_test.dart b/runtime/tests/vm/dart_2/regress_b_216834909_test.dart new file mode 100644 index 00000000000..65dd27c3c64 --- /dev/null +++ b/runtime/tests/vm/dart_2/regress_b_216834909_test.dart @@ -0,0 +1,36 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// 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. +// +// SharedObjects=ffi_test_functions + +// Regression test for b/216834909. +// +// Check that subprocess spawning implementation uses _exit rather than exit on +// paths which terminate fork child without exec-ing. + +// @dart = 2.9 + +import 'dart:async'; +import 'dart:ffi'; +import 'dart:io'; +import 'dart:isolate'; + +import "package:expect/expect.dart"; +import '../../../../tests/ffi/dylib_utils.dart'; + +final ffiTestFunctions = dlopenPlatformSpecific('ffi_test_functions'); + +final setAtExit = + ffiTestFunctions.lookupFunction( + 'Regress216834909_SetAtExit'); + +main(List args) async { + // We only care about platforms which use fork/exec. + if (!Platform.isLinux && !Platform.isAndroid && !Platform.isMacOS) { + return; + } + setAtExit(1); // Install at exit handler. + await Process.start('true', [], mode: ProcessStartMode.detached); + setAtExit(0); // Clear at exit handler. +}