5
\$\begingroup\$

I am trying to execute a Bash command, get the pid, read the (error) output and optionally pass input.

I am still quite new at C++ and I would like to know if this is a correct & safe implementation for use in real-world applications?

The target operating systems are MacOS 12.5 & Ubuntu (server) 20.04.

The target C++ version is C++20 (aka C++2a).

#include <iostream>
#include <unistd.h>
#include <array>
#include <string>

int main() {
    
    // Vars.
    const char* command = "echo Hello World!";
    const char* input = "";
    const int READ_END = 0;
    const int WRITE_END = 1;
    int infd[2] = {0, 0};
    int outfd[2] = {0, 0};
    int errfd[2] = {0, 0};

    // Cleanup func.
    auto cleanup = [&]() {
        ::close(infd[READ_END]);
        ::close(infd[WRITE_END]);
        ::close(outfd[READ_END]);
        ::close(outfd[WRITE_END]);
        ::close(errfd[READ_END]);
        ::close(errfd[WRITE_END]);
    };

    // Pipes.
    auto rc = ::pipe(infd);
    if(rc < 0) {
        throw std::runtime_error(std::strerror(errno));
    }
    rc = ::pipe(outfd);
    if(rc < 0) {
        ::close(infd[READ_END]);
        ::close(infd[WRITE_END]);
        throw std::runtime_error(std::strerror(errno));
    }
    rc = ::pipe(errfd);
    if(rc < 0) {
        ::close(infd[READ_END]);
        ::close(infd[WRITE_END]);
        ::close(outfd[READ_END]);
        ::close(outfd[WRITE_END]);
        throw std::runtime_error(std::strerror(errno));
    }

    // Parent.
    auto pid = fork();
    if(pid > 0) {
        ::close(infd[READ_END]);    // Parent does not read from stdin
        ::close(outfd[WRITE_END]);  // Parent does not write to stdout
        ::close(errfd[WRITE_END]);  // Parent does not write to stderr
        if(::write(infd[WRITE_END], input, strlen(input)) < 0) {
            throw std::runtime_error(std::strerror(errno));
        }
        ::close(infd[WRITE_END]);
    }

    // Child.
    else if(pid == 0) {
        ::dup2(infd[READ_END], STDIN_FILENO);
        ::dup2(outfd[WRITE_END], STDOUT_FILENO);
        ::dup2(errfd[WRITE_END], STDERR_FILENO);

        ::close(infd[WRITE_END]);   // Child does not write to stdin
        ::close(outfd[READ_END]);   // Child does not read from stdout
        ::close(errfd[READ_END]);   // Child does not read from stderr

        ::execl("/bin/bash", "bash", "-c", command, nullptr);
        ::exit(EXIT_SUCCESS);
    }

    // Error.
    if(pid < 0) {
        cleanup();
        throw std::runtime_error("Failed to fork");
    }

    // Wait at pid.
    int status = 0;
    ::waitpid(pid, &status, 0);

    // Read output.
    std::string output;
    std::string error;
    std::array<char, 256> buffer;
    ssize_t bytes = 0;
    while ((bytes = ::read(outfd[READ_END], buffer.data(), buffer.size())) > 0) {
        output.append(buffer.data(), bytes);
    }
    while ((bytes = ::read(errfd[READ_END], buffer.data(), buffer.size())) > 0) {
        error.append(buffer.data(), bytes);
    }

    // Get exit status.
    if(WIFEXITED(status)) {
        status = WEXITSTATUS(status);
    }

    // Cleanup.
    cleanup();
    
    std::cout << "PID: " << pid << "\n";
    std::cout << "STATUS: " << status << "\n";
    std::cout << "OUTPUT: " << output << "\n";
    std::cout << "ERROR: " << error << "\n";
    
}
\$\endgroup\$
4
  • \$\begingroup\$ It's too late to post additional code for review once you have answers. I recommend you incorporate improvements into your code and then post a follow-up question. \$\endgroup\$ Commented Sep 1, 2022 at 10:45
  • \$\begingroup\$ @TobySpeight - Allright, so it is allowed to edit the already posted code? Since i've had negative comments about this in the past. \$\endgroup\$ Commented Sep 1, 2022 at 10:47
  • 3
    \$\begingroup\$ No, as that can cause answers to become invalid. Once you have one or more answers, don't change the code being reviewed! And try to minimise other edits. But we do welcome follow-up questions that incorporate the advice you have received. See I improved my code based on the reviews. What next? \$\endgroup\$ Commented Sep 1, 2022 at 10:52
  • \$\begingroup\$ Fun question. Good answer. \$\endgroup\$ Commented Sep 11, 2022 at 18:02

1 Answer 1

2
\$\begingroup\$

This is a reasonably good start.


You definitely want to use a function, rather than putting everything in main(). That will force you to think about how to return the output and error strings and the exit status (returning the PID seems pointless after the process has exited).

It might be better to pass a couple of std::ostream& as arguments (perhaps default to null streams), rather than returning strings. That could allow the calling program to handle output as it is produced, rather than waiting until the child has finished.


The cleanup is scattered all over the function - seriously consider wrapping those pipe file-descriptors in RAII objects so that they clean up themselves whenever the function throws or returns.


The structure here is a bit strange:

auto pid = fork();
if(pid > 0) {
    ⋮ 
}

// Child.
else if(pid == 0) {
    ⋮ 
}

// Error.
if(pid < 0) {
    ⋮ 
    cleanup();
    throw std::runtime_error("Failed to fork");
}

That final if could be an else (and I'd probably test that first, to get the error handling out of the way). The only negative value that fork() is documented to return is -1, so we could even use a switch here.

It's odd that the exception thrown on fork failure discards the useful information in errno, that we keep in other failure paths.


This is definitely wrong:

    ::execl("/bin/bash", "bash", "-c", command, nullptr);
    ::exit(EXIT_SUCCESS);

If execl() returns, it means it failed, so we should be exiting with a failure status.


I'm not sure that leaving status = 0 when the program exited by signal is a good choice. And if the process hasn't terminated (remember that wait() returns when the child is stopped, for example), we probably want to loop.


We can have a deadlock with programs that produce more output, because we wait for the process to exit before we start reading its output - but the process may be blocked on its output pipe. We'll need to read output as it is produced (using select() to choose which stream to read from, and periodically wait() for it with the WNOHANG option set to see if it has terminated).

\$\endgroup\$
9
  • \$\begingroup\$ Thanks, great answer! 1: I indeed have everything inside a function (struct actually with async options). 2: I corrected the strange structure, I overlooked it :P. 3: About the failure status; does execl return the exit status of the process or just an internal failure status from execl? Since the exit status does not work (is always 0) when testing a custom script that exits with status 1. 4: Could you create a mini example about the deadlock blocked pipe for reading using select as the output is produced. Since I do not completely understand this. I will take a look at the docs as well. \$\endgroup\$ Commented Sep 1, 2022 at 10:38
  • \$\begingroup\$ Should I post my non reproducable real code? Since this contains some fixes you suggested. Edit: just posted it. Edit: Okay it does not seem to be allowed :s. \$\endgroup\$ Commented Sep 1, 2022 at 10:41
  • \$\begingroup\$ What execl() returns is well documented in the execve() man page: "On success, execve() does not return, on error -1 is returned, and errno is set to indicate the error." It can't return the exit status of the process, as the process hasn't exited! \$\endgroup\$ Commented Sep 1, 2022 at 10:48
  • \$\begingroup\$ Simple blocked-pipe example: just cat any sufficiently large file (larger than a pipe's capacity - e.g. for Linux, anything bigger than 64K should be sufficient) \$\endgroup\$ Commented Sep 1, 2022 at 10:49
  • \$\begingroup\$ Allright thanks, any link on where I can learn about the pipe capacity? Since I do not quite understand this yet. The same for the buffer size which is currently 256, does this mean every buffer batch has a max of 256 bytes? And the pipe's capacity is system defined? \$\endgroup\$ Commented Sep 1, 2022 at 10:54

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.