[vm] Avoid deadlocks when launching subprocesses

Forked process should use _exit() rather than exit()
when exiting without exec-ing otherwise it risks
to hit an issue (e.g. deadlock) in an atexit handler.

Additionally `man fork` states:

> After a fork() in a multithreaded program, the child can
> safely call only async-signal-safe functions (see
> signal-safety(7)) until such time as it calls execve(2).

_exit is on the list of async-signal-safe functions, but
exit is not.

Fixes b/216834909

TEST=runtime/tests/vm/dart{,_2}/regress_b_216834909_test.dart

Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-android-release-arm64c-try,vm-kernel-mac-release-x64-try
Change-Id: Ia67f23825fc0ee1c1918faf2d4ef3b81033263eb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/241608
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>

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

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<char* const*>(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);
     }
   }
 

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<char* const*>(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);
   }
 

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<char* const*>(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) {

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<Void Function(Int64), void Function(int)>(
+        'Regress216834909_SetAtExit');
+
+main(List<String> 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.
+}

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<Void Function(Int64), void Function(int)>(
+        'Regress216834909_SetAtExit');
+
+main(List<String> 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.
+}