std.process.Child.collectOutput: potential for crash or memory leak

[KilianHanich]

Zig Version

0.13.0

Steps to Reproduce and Observed Behavior

std.process.Child.collectOutput treats the stdout and stderr parameters as output parameters which can just be overwritten (as seen in line 360 and 361) but also relies on data from them (as seen in line 341 and 342 and in line 347).

This means that a user must initialize the parameters, but never actually can put stuff into it because that would lead to a memory leak if they don't also have a copy of the parameter or don't empty the ArrayList first.

This means both of these tests fail:

test "leak" {
    // setup
    const argv = [_][]const u8{ "echo", "Hello" };
    var child = std.process.Child.init(&argv, std.testing.allocator);
    child.stderr_behavior = .Pipe;
    child.stdout_behavior = .Pipe;
    try child.spawn();

    // actual test
    var stdout = std.ArrayList(u8).init(std.testing.allocator);
    defer stdout.deinit();
    try stdout.append('a');
    var stderr = std.ArrayList(u8).init(std.testing.allocator);
    defer stderr.deinit();
    //yes, a bit overkill with max_output_bytes, but that's not the point here
    try child.collectOutput(&stdout, &stderr, 50);

    // cleanup
    _ = try child.wait();
}

test "undef error" {
    // setup
    const argv = [_][]const u8{"echo", "Hello"};
    var child = std.process.Child.init(&argv, std.testing.allocator);
    child.stderr_behavior = .Pipe;
    child.stdout_behavior = .Pipe;
    try child.spawn();

    // actual test
    var stdout: std.ArrayList(u8) = undefined;
    var stderr: std.ArrayList(u8) = undefined;
    // at least on my end (x86-64) I get reliably a "General protection exception (no address available)"
    // which has to do with the MMU because thanks to undefined it tries to access an area where the upper 16 bits are set
    try child.collectOutput(&stdout, &stderr, 50);
    defer stdout.deinit();
    defer stderr.deinit();

    // cleanup
    _ = try child.wait();
}

Expected Behavior

There are a few (mutually exclusive) ways I would this function imagine to behave, and some which would improve on the current behaviour. These are:

• The function deinitializes the parameters before overwriting them.
• The function doesn't rely on state of the parameters at all, so users can pass in functions which are "initialized" with undefined.
• The function has an allocator interface as a parameters instead and returns a (anonymous) struct with the newly created ArrayLists (I would prefer this one as a user of the function).
• The function appends the output to the given ArrayLists instead of overwriting them.