Skip to content

spawn: Don't set a search path if we don't want to search PATH

Simon McVittie requested to merge wip/avoid-searching-path into master

  • spawn: Don't set a search path if we don't want to search PATH

    do_exec() and g_execute() rely on being passed a NULL search path if we intend to avoid searching the PATH, but since the refactoring in commit 62ce66d4, this was never done. This resulted in some spawn calls searching the PATH when it was not intended.

    Spawn calls that go through the posix_spawn fast-path were unaffected.

    The deprecated gtester utility, as used in GTK 3, relies on the ability to run an executable from the current working directory by omitting the G_SPAWN_SEARCH_PATH flag. This mostly worked, because our fallback PATH ends with ".". However, if an executable of the same name existed in /usr/bin or /bin, it would run that instead of the intended test: in particular, GTK 3's build-time tests failed if ImageMagick happens to be installed, because gtester would accidentally run display(1) instead of testsuite/gdk/display.

    Fixes: 62ce66d4 "gspawn: Don’t use getenv() in async-signal-safe context"
    Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977961

  • Add test coverage for G_SPAWN_SEARCH_PATH

    For manual test coverage that would reproduce the bug fixed in !1902 (merged), copy /bin/true (or any other harmless executable) to /usr/bin/spawn-test-helper.

  • spawn: prefer allocating buffers on stack for small sizes to avoid valgrind leaks

    From: @thaller

    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.

    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.

    [smcv: Cosmetic adjustments to address review comments from pwithnall]

  • Expand test coverage for G_SPAWN_SEARCH_PATH

v5: Add @thaller's patch to avoid the buffer used for this showing up as a memory leak under valgrind (we have no idea why this is newly showed up as a problem now).

v6/v7: Fix @pwithnall's cosmetic review comments.

v8: Attempt to add trivial test coverage for the heap-allocation and PATH-not-set code paths. Note that I'm not really verifying that exactly the right thing is done for the PATH-not-set code path, because the only way to make that happen would be to put an executable under our control in /usr/bin, which we can't actually do.

/cc @pwithnall

Edited by Simon McVittie

Merge request reports