[PATCH 0/1] MR4468: ntdll: Use posix_spawn() to start the server.

[Brendan Shanks (＠bshanks)]

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4468

[PATCH 1/1] ntdll: Use posix_spawn() to start the server.

[Brendan Shanks]

From: Brendan Shanks <bshanks(a)codeweavers.com>

---
 dlls/ntdll/unix/loader.c | 73 +++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 24 deletions(-)

diff --git a/dlls/ntdll/unix/loader.c b/dlls/ntdll/unix/loader.c
index aa120ae38ec..a43c87f735c 100644
--- a/dlls/ntdll/unix/loader.c
+++ b/dlls/ntdll/unix/loader.c
@@ -30,6 +30,7 @@
 #include <stdarg.h>
 #include <stdio.h>
 #include <signal.h>
+#include <spawn.h>
 #include <string.h>
 #include <stdlib.h>
 #include <sys/types.h>
@@ -69,7 +70,6 @@
 # include <mach/mach_error.h>
 # include <mach-o/getsect.h>
 # include <crt_externs.h>
-# include <spawn.h>
 # ifndef _POSIX_SPAWN_DISABLE_ASLR
 #  define _POSIX_SPAWN_DISABLE_ASLR 0x0100
 # endif
@@ -77,6 +77,7 @@
 #ifdef __ANDROID__
 # include <jni.h>
 #endif
+extern char **environ;
 
 #include "ntstatus.h"
 #define WIN32_NO_STATUS
@@ -546,47 +547,72 @@ NTSTATUS exec_wineloader( char **argv, int socketfd, const pe_image_info_t *pe_i
 
 
 /***********************************************************************
- *           exec_wineserver
+ *           spawn_wineserver
  *
- * Exec a new wine server.
+ * Spawn a new wine server.
  */
-static void exec_wineserver( char **argv )
+static int spawn_wineserver( pid_t *pid, char **argv )
 {
-    char *path;
+    char *path, *pathcopy;
+    int ret;
 
     if (build_dir)
     {
         if (!is_win64)  /* look for 64-bit server */
         {
-            char *loader = realpath_dirname( build_path( build_dir, "loader/wine64" ));
+            char *loader_relative = build_path( build_dir, "loader/wine64" );
+            char *loader = realpath_dirname( loader_relative );
+            free( loader_relative );
             if (loader)
             {
                 argv[0] = build_path( loader, "../server/wineserver" );
-                execv( argv[0], argv );
+                free( loader );
+                ret = posix_spawn( pid, argv[0], NULL, NULL, argv, environ );
+                free( argv[0] );
+                if (!ret)
+                    return 0;
             }
         }
         argv[0] = build_path( build_dir, "server/wineserver" );
-        execv( argv[0], argv );
-        return;
+        ret = posix_spawn( pid, argv[0], NULL, NULL, argv, environ );
+        free( argv[0] );
+        return ret;
     }
 
     argv[0] = build_path( bin_dir, "wineserver" );
-    execv( argv[0], argv );
+    ret = posix_spawn( pid, argv[0], NULL, NULL, argv, environ );
+    free( argv[0] );
+    if (!ret)
+        return 0;
 
     argv[0] = getenv( "WINESERVER" );
-    if (argv[0]) execv( argv[0], argv );
+    if (argv[0])
+    {
+        ret = posix_spawn( pid, argv[0], NULL, NULL, argv, environ );
+        if (!ret)
+            return 0;
+    }
 
-    if ((path = getenv( "PATH" )))
+    if ((pathcopy = getenv( "PATH" )) && ((pathcopy = strdup( pathcopy ))))
     {
-        for (path = strtok( strdup( path ), ":" ); path; path = strtok( NULL, ":" ))
+        for (path = strtok( pathcopy, ":" ); path; path = strtok( NULL, ":" ))
         {
             argv[0] = build_path( path, "wineserver" );
-            execvp( argv[0], argv );
+            ret = posix_spawn( pid, argv[0], NULL, NULL, argv, environ );
+            free( argv[0] );
+            if (!ret)
+            {
+                free( pathcopy );
+                return 0;
+            }
         }
+        free( pathcopy );
     }
 
     argv[0] = build_path( BINDIR, "wineserver" );
-    execv( argv[0], argv );
+    ret = posix_spawn( pid, argv[0], NULL, NULL, argv, environ );
+    free( argv[0] );
+    return ret;
 }
 
 
@@ -604,15 +630,14 @@ void start_server( BOOL debug )
     if (!started)
     {
         int status;
-        int pid = fork();
-        if (pid == -1) fatal_error( "fork: %s", strerror(errno) );
-        if (!pid)
-        {
-            argv[1] = debug ? debug_flag : NULL;
-            argv[2] = NULL;
-            exec_wineserver( argv );
-            fatal_error( "could not exec wineserver\n" );
-        }
+        pid_t pid;
+
+        argv[1] = debug ? debug_flag : NULL;
+        argv[2] = NULL;
+
+        status = spawn_wineserver( &pid, argv );
+        if (status) fatal_error( "could not spawn wineserver: %s", strerror(errno) );
+
         waitpid( pid, &status, 0 );
         status = WIFEXITED(status) ? WEXITSTATUS(status) : 1;
         if (status == 2) return;  /* server lock held by someone else, will retry later */
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/4468

[Alexandre Julliard (＠julliard)]

That doesn't look like an improvement. Is it really necessary?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4468#note_54135

[Chip Davis (＠cdavis5e)]

On Tue Nov 28 23:08:57 2023 +0000, Alexandre Julliard wrote:
> That doesn't look like an improvement. Is it really necessary?
> It is on Mac OS, where `fork(2)` doesn't play too well with many system frameworks.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4468#note_54176

[Brendan Shanks (＠bshanks)]

On Tue Nov 28 23:08:57 2023 +0000, Chip Davis wrote:
> It is on Mac OS, where `fork(2)` doesn't play too well with many system frameworks.
> `fork()` is particularly slow on macOS, this is negligible for a single prefix but adds up for an app like CrossOver which is managing many bottles and starting them to retrieve settings, etc.

Also I hadn't considered it, but Chip's point is a good one when Wine is running inside a Mac Cocoa app (like CrossOver's launchers).

The error handling and many calls to `posix_spawn()` do complicate the code, I'll see if it would be simpler to pre-generate the possible `wineserver` paths and then loop through them until one succeeds.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4468#note_54188