Skip to content

Error handling may call waitid twice (double-free issue) #131

@jakepetroules

Description

@jakepetroules

Consider the following code:

// Communicate child pid back
*pid = childPid;
// Read from the pipe until pipe is closed
// either due to exec succeeds or error is written
while (1) {
int childError = 0;
ssize_t read_rc = read(pipefd[0], &childError, sizeof(childError));
if (read_rc == 0) {
// exec worked!
close(pipefd[0]);
return 0;
}
// if we reach this point, exec failed.
// Since we already have the child pid (fork succeed), reap the child
// This mimic posix_spawn behavior
siginfo_t info;
waitid(P_PID, childPid, &info, WEXITED);
if (read_rc > 0) {
// Child exec failed and reported back
close(pipefd[0]);
return childError;
} else {
// Read failed
if (errno == EINTR) {
continue;
} else {
close(pipefd[0]);
return errno;
}
}
}

The following could happen:

  • read_rc returns -1
  • waitid is called to wait for exit and consume the zombie, succeeds (doesn't modify errno)
  • errno is EINTR, from the read call
  • continue is called to restart the loop
  • read is called again, and returns nonzero
  • waitid is called again, but now childPid may point to a different process

I think we need to structure the code like this:

// Communicate child pid back
*pid = childPid;
*pidfd = _pidfd;
// Read from the pipe until pipe is closed
// either due to exec succeeds or error is written
while (1) {
    int childError = 0;
    ssize_t read_rc = read(pipefd[0], &childError, sizeof(childError));
    if (read_rc == 0) {
        // exec worked!
        close(pipefd[0]);
        return 0;
    }
    int read_errno = errno;
    if (read_rc < 0 && read_errno == EINTR) {
        continue;
    }
    // if we reach this point, exec failed.
    // Since we already have the child pid (fork succeed), reap the child
    // This mimic posix_spawn behavior
    while (1) {
        siginfo_t info;
        int waitid_result = waitid(P_PID, childPid, &info, WEXITED);
        if (waitid_result == -1 && errno == EINTR) {
            continue;
        } else {
            break;
        }
    }

    close(pipefd[0]);
    if (read_rc > 0) {
        // Child exec failed and reported back
        return childError;
    } else {
        return read_errno;
    }
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions