Bug 23654 – execv_: toAStringz: memory corruption

// // tassnd.d // 2023-01-24 stvo // function toAStringz taken from dmd2/src/phobos/std/process.d // of DMD64 D Compiler v2.102.0-rc.1 // compile: dmd -checkaction=context -unittest -main -run tassnd.d // module tassnd; import std.conv; import std.stdio; import core.stdc.stdlib; import std.exception; import core.exception : OutOfMemoryError; private void toAStringz(in string[] a, const(char)**az) { import std.string : toStringz; foreach (string s; a) { *az++ = toStringz(s); } *az = null; } bool data_are_the_same (const string [] a, const char **b) { foreach (i, s; a) { auto bs = b [i].to!string; // if (i < 3) // writefln!"%s %s %s" (i, s, bs); if (s != bs) { writefln!"i = <%d> a <%s> b <%s>" (i, s, bs); return false; } } return true; } unittest { immutable sizlist = [8, 16, 32, 64, 128]; foreach (p; sizlist) { size_t s = 1024 * p; writeln (s); auto argv = new string [s]; argv [0] = "ppp"; argv [1] = s.to!string; argv [2 .. $] = "X"; // // fragment taken from private int execv_ (...) // auto argv_ = cast(const(char)**)core.stdc.stdlib.malloc((char*).sizeof * (1 + argv.length)); enforce!OutOfMemoryError(argv_ !is null, "Out of memory in std.process."); scope(exit) core.stdc.stdlib.free(argv_); toAStringz(argv, argv_); // // end fragment // assert (data_are_the_same (argv, argv_)); } } $ dmd -checkaction=context -unittest -main -run tassnd.d 8192 16384 32768 i = <12156> a <X> b <@> tassnd.d(58): [unittest] false != true 1/1 modules FAILED unittests

(In reply to kdevel from comment #0) > private void toAStringz(in string[] a, const(char)**az) > { > import std.string : toStringz; > foreach (string s; a) > { > *az++ = toStringz(s); > } > *az = null; > auto argv_ = cast(const(char)**)core.stdc.stdlib.malloc((char*).sizeof * > (1 + argv.length)); It seems to be GC-related. The unittest passes, if - the GC is disabled, or - the memory is allocated with new: $ dmd -checkaction=context -unittest -main -run tassnd-no-gc.d 8192 16384 32768 65536 131072 1 modules passed unittests $ dmd -checkaction=context -unittest -main -run tassnd-no-malloc.d 8192 16384 32768 65536 131072 1 modules passed unittests

Created attachment 1866 GC disabled

Created attachment 1867 use new for allocation (no malloc)

Regression, introduced here: https://github.com/dlang/phobos/commit/6302257b0cdc5d171511cc6f1566956ff11b09c5

Created attachment 1868 Patch (v0) against process.d Could not yet test this code. That is how I would implement those D wrappers.

Doesn't fork break the GC heap? If you want to allocate from GC after fork, it may not work.

Created attachment 1888 D-Program execv-recur.d demonstrating the memory corruption // // execv-recur.d // 2023-08-29 stvo // // disable the GC with // // DRT_GCOPT=disable:1 ./execv-recur 1 // // this program then expectedly reports // // rc = <-1> errno = <7> strerror = <Argument list too long> // // If run with enabled GC // // ./execv-recur 1 // // the memory gets corrupted and the program reports // // [email protected](49): exp <4> actual <131072> //

(In reply to anonymous4 from comment #6) > Doesn't fork break the GC heap? If you want to allocate from GC after fork, > it may not work. The corruption does not require a previous call to fork.

(In reply to anonymous4 from comment #6) > Doesn't fork break the GC heap? If you want to allocate from GC after fork, > it may not work. fork *can* hang if another thread is holding the GC lock. But we can potentially work around this if we take the GC lock before calling fork. But using the GC has always been a part of this, it's not new. The toAStringz is using the GC. The problem with the original change is it moved the allocation of the array (which points at GC memory) from a scannable place to a non-scannable place.

As I understand the code initially didn't touch GC (because it can fail after fork or to match posix guarantees) then regressed to use GC.

If you look at that commit from 2015, it was using alloca before, and now uses malloc. But the issue is that the `toAStringz` which allocates an array of C strings using the GC. This was the case before the 2015 change, and is still the case now. So the GC was always used. The issue that happened when moving to malloc is that the GC could clean up those string items before they were sent to execv. When it was using alloca, that went on the stack, and stacks are scanned. The simple solution is to put that array on the GC. Given that for at least 8 years, and probably more, using the GC was fine when forking/execing a process, most likely it's either just fine, or it hangs so infrequently that nobody has complained about it. Using the GC for the string array along with all the strings is at least the same risk as just allocating the strings using the GC.

(In reply to kdevel from comment #5) > Created attachment 1868 [details] > Patch (v0) against process.d > > Could not yet test this code. That is how I would implement those D wrappers. The test is rather simple. 1. Locate process.d (locate src/phobos/std/process.d) and copy it to the current working directory. 2. Download, compile, execute execv-recur.d (4th attachment) $ dmd execv-recur.d process.d $ ./execv-recur 1 [...] self <./execv-recur> newargs [0, 1] <./execv-recur> <131072> dump: <2> <4> <2> <32> <2> <4> <2> [email protected](49): exp <4> actual <131072> [...] 3. Download the Patch (3rd attachment) apply, recompile, execute: $ patch -i issue-23654.v0.patch patching file process.d Hunk #1 succeeded at 4309 (offset 50 lines). Hunk #2 succeeded at 4374 (offset 50 lines). $ dmd execv-recur.d process.d $ ./execv-recur 1 [...] self <./execv-recur> newargs [0, 1] <./execv-recur> <262144> rc = <-1> errno = <7> strerror = <Argument list too long> This is the expected outcome.

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/phobos/issues/10514 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB