[glib/wip/avoid-searching-path-2-66: 3/3] spawn: prefer allocating buffers on stack for small sizes to avoid valgrind leaks

[Simon McVittie <smcv src gnome org>]

commit 6dad0eb8040e23b1779600fa5346e20123d647bb
Author: Thomas Haller <thaller redhat com>
Date:   Fri Jan 29 12:26:00 2021 +0100

    spawn: prefer allocating buffers on stack for small sizes to avoid valgrind leaks
    
    We preallocate buffers that are used after forked. That is because
    malloc()/free() are not async-signal-safe and must not be used between
    fork() and exec().
    
    However, for the child process that exits without fork, valgrind wrongly
    reports these buffers as leaked.
    That can be suppressed with "--child-silent-after-fork=yes", but it is
    cumbersome.
    
    ==3989269== 24 bytes in 1 blocks are definitely lost in loss record 2 of 11
    ==3989269==    at 0x4839809: malloc (vg_replace_malloc.c:307)
    ==3989269==    by 0x489EC28: g_malloc (gmem.c:106)
    ==3989269==    by 0x48E60F4: fork_exec_with_fds (gspawn.c:1879)
    ==3989269==    by 0x48E6C1A: fork_exec_with_pipes (gspawn.c:2214)
    ==3989269==    by 0x48E75CE: g_spawn_async_with_pipes (gspawn.c:792)
    ==3989269==    by 0x48E768F: g_spawn_async (gspawn.c:257)
    ==3989269==    by 0x401093: main (test-mr1902.c:16)
    ==3989269==
    ==3989269== 126 bytes in 1 blocks are definitely lost in loss record 9 of 11
    ==3989269==    at 0x4839809: malloc (vg_replace_malloc.c:307)
    ==3989269==    by 0x489EC28: g_malloc (gmem.c:106)
    ==3989269==    by 0x48E62F0: fork_exec_with_fds (gspawn.c:1867)
    ==3989269==    by 0x48E6C1A: fork_exec_with_pipes (gspawn.c:2214)
    ==3989269==    by 0x48E75CE: g_spawn_async_with_pipes (gspawn.c:792)
    ==3989269==    by 0x48E768F: g_spawn_async (gspawn.c:257)
    ==3989269==    by 0x401093: main (test-mr1902.c:16)
    ==3989269==
    
    Work around by trying to allocate the buffers on the stack. At
    least in the common cases where the pointers are small enough
    so that we can reasonably do that.
    
    If the buffers happen to be large, we still allocate them on the heap
    and the problem still happens. Maybe we could have also allocated them
    as thread_local, but currently glib doesn't use that.

 glib/gspawn.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)
---
diff --git a/glib/gspawn.c b/glib/gspawn.c
index 0b9473022..40749c025 100644
--- a/glib/gspawn.c
+++ b/glib/gspawn.c
@@ -1753,8 +1753,10 @@ fork_exec_with_fds (gboolean              intermediate_child,
   gint status;
   const gchar *chosen_search_path;
   gchar *search_path_buffer = NULL;
+  gchar *search_path_buffer_heap = NULL;
   gsize search_path_buffer_len = 0;
   gchar **argv_buffer = NULL;
+  gchar **argv_buffer_heap = NULL;
   gsize argv_buffer_len = 0;
 
 #ifdef POSIX_SPAWN_AVAILABLE
@@ -1848,7 +1850,16 @@ fork_exec_with_fds (gboolean              intermediate_child,
   if (chosen_search_path != NULL)
     {
       search_path_buffer_len = strlen (chosen_search_path) + strlen (argv[0]) + 2;
-      search_path_buffer = g_malloc (search_path_buffer_len);
+      if (search_path_buffer_len < 4000)
+        {
+          /* prefer small stack allocations to avoid valgrind leak warnings in forked child. */
+          search_path_buffer = g_alloca (search_path_buffer_len);
+        }
+      else
+        {
+          search_path_buffer_heap = g_malloc (search_path_buffer_len);
+          search_path_buffer = search_path_buffer_heap;
+        }
     }
 
   if (search_path || search_path_from_envp)
@@ -1860,12 +1871,21 @@ fork_exec_with_fds (gboolean              intermediate_child,
    * script_execute() has to be called later on, it can build a wrapper argv
    * array in this buffer. */
   argv_buffer_len = g_strv_length (argv) + 2;
-  argv_buffer = g_new (gchar *, argv_buffer_len);
+  if (argv_buffer_len < 4000 / sizeof (gchar *))
+    {
+      /* prefer small stack allocations to avoid valgrind leak warnings in forked child. */
+      argv_buffer = g_newa(gchar *, argv_buffer_len);
+    }
+  else
+    {
+      argv_buffer_heap = g_new (gchar *, argv_buffer_len);
+      argv_buffer = argv_buffer_heap;
+    }
 
   if (!g_unix_open_pipe (child_err_report_pipe, pipe_flags, error))
     {
-      g_free (search_path_buffer);
-      g_free (argv_buffer);
+      g_free (search_path_buffer_heap);
+      g_free (argv_buffer_heap);
       return FALSE;
     }
 
@@ -2112,8 +2132,8 @@ fork_exec_with_fds (gboolean              intermediate_child,
       close_and_invalidate (&child_err_report_pipe[0]);
       close_and_invalidate (&child_pid_report_pipe[0]);
 
-      g_free (search_path_buffer);
-      g_free (argv_buffer);
+      g_free (search_path_buffer_heap);
+      g_free (argv_buffer_heap);
 
       if (child_pid)
         *child_pid = pid;
@@ -2147,8 +2167,8 @@ fork_exec_with_fds (gboolean              intermediate_child,
   close_and_invalidate (&child_pid_report_pipe[0]);
   close_and_invalidate (&child_pid_report_pipe[1]);
 
-  g_free (search_path_buffer);
-  g_free (argv_buffer);
+  g_free (search_path_buffer_heap);
+  g_free (argv_buffer_heap);
 
   return FALSE;
 }