design of Popen arguments promotes unsafe use

[klosworks]

This is a design issue. Basically there is a problem with definitions like this:

struct output
{
  output(int fd): wr_ch_(fd) {}

  output (FILE* fp):output(fileno(fp)) { assert(fp); }

  output(const char* filename) {
    int fd = open(filename, O_APPEND | O_CREAT | O_RDWR, 0640);
    if (fd == -1) throw OSError("File not found: ", errno);
    wr_ch_ = fd;
}
(...)

The problem is that output is not a RAII type that will close the file if not used, but rather a thin wrapper. This is not good for a type that is just a syntactic sugar for parameter passing. The thing is the user will expect to be able to prepare the arguments before the Popen call. If they are prepared but the Popen call is missed, this will leave dangling file handles. Imagine code like this:

output o;
if(file_given)
    o = output{file_name};
if(condition)
{
    auto p = Popen({ "grep", "f" }, o, input{ PIPE });
    (...)
}

If condition fails, the code will leave a dangling open file. The solution is to avoid opening files in the Popen argument types, and just store the filenames in them. The actual file opening should be done by Popen itself. Alternatively, there could be a RAII solution here, but it doesn't look as clean (it isn't needed to open the files until the actual Popen call is made).

[kuvaldini]

@klosworks , good notice . I did not use this lib in a such way. If you know a problem and the solution it would be nice to review a pull request.

[arun11299]

Good point. This did not catch my attention. Sorry for that. @klosworks If you can make a pull request, that would be highly appreciated. Otherwise I will try to do this in my free time.

Thanks.