Re: Issue with fcntl FD_CLOEXEC and execve

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Please keep me in CC as I'm not subscribed to the mailing list]


Hello,

Thanks for your quick answer; sorry for the late response but I had meetings today (I'm in timezone UTC+2).

I tried to reproduce the bug in a small C app but couldn't, instead I got SIGPIPE from C program. However I could successfully reliably reproduce the bug on my C++ project (tested 10 times and it locked 10/10 times) using both cat and a custom binary. I wonder if it is not part of a third party I'm using such as Google Test or ZLib (these are the only two third parties I'm using).

I can point you to the key parts of the C++ program that always reproduces the same bug. I even introduced a work-arround which does work but gets rid of the error testing for execve:

https://github.com/BlockProject3D/Framework/blob/ProcessManagement/Base/src/Framework/System/Process.cpp > This is the main class that performs fork, pipe, read, write and execve, key lines are 198-262, 267-282 and 380-414. The code is already tested so you can assume the C++ String and Array parts do work as execve actually calls the correct executable with the correct environment and arguments (gdb + gtest tested). You don't need to look inside #ifdef WINDOWS as it's only for WinAPI. On line 57 you can switch the define OFF to cause the lock-forever read bug.

https://github.com/BlockProject3D/Framework/blob/ProcessManagement/Tests/src/System/Process.cpp > This is the code that interfaces with Google Test in order to unit test the entire Framework. You can find the tests that reproduces the bug at line 163-182, 187-206 and 212-226.

The Framework can be compiled with help of CMake (sudo apt install cmake) and conan (sudo pip3 install conan). The compiler does not matter; use whatever compiler you prefer the most; architecture must be either x86, x86_64, arm & derived or arm64 & derived and system must be any distribution of Linux that has support for C++11 minimum.

I also join my attempt at reproducing the bug in a smaller C file. To compile the small file use whatever compiler you prefer the most.

PS: As you can probably noticed I fixed my email address (well actually just added one more email address on the private linux box)...

I hope you can help me,

Thank you,

Yuri Edward


On 6/20/20 8:53 AM, Florian Weimer wrote:
* postmaster:

The pipe works fine and the process can correctly transmit data back to the
master. The only issue appears when trying to run input pump based
applications like cat, grep, etc. When running these applications the pipe
write end which is marked FC_CLOEXEC should be closed when execve is
triggered, however it is not, but execve still succeeded.
When the target process to execve is not an input pump (uname, ls, .) no
issue occurs and the pipe is correctly closed when execve is triggered and
succeeds.

That is a problem as my master process expects the blocking read to return
with either cause of pipe file descriptor closed or some data written. In
this case when the target process is an input pump the master process never
gets notified of file descriptor closed and by extension locks forever.
Do you have a minimal but complete example that shows the problem?

You cannot really make standard input a CLOEXEC descriptor.  If you do
not want subprocesses to use the original standard input, you need to
replace the descriptor with /dev/null before the execve call (but
already in the subprocess).
#include <fcntl.h>
#include <string.h>
#include <unistd.h>
#include <wait.h>
#include <stdio.h>
#include <stdlib.h>

#define PIPE_WRITE 1
#define PIPE_READ 0

#define BP_IGNORE(v) if (v) {} //Required to workarround GCC bug

void CleanupHandles(int fdStdOut[2], int fdStdErr[2], int fdStdIn[2], int commonfd[2])
{
    for (int i = 0; i != 2; ++i)
    {
        if (fdStdOut[i] != -1)
            close(fdStdOut[i]);
    }
    for (int i = 0; i != 2; ++i)
    {
        if (fdStdErr[i] != -1)
            close(fdStdErr[i]);
    }
    for (int i = 0; i != 2; ++i)
    {
        if (fdStdIn[i] != -1)
            close(fdStdIn[i]);
    }
    for (int i = 0; i != 2; ++i)
    {
        if (commonfd[i] != -1)
            close(commonfd[i]);
    }
}

void ProcessWorker(int fdStdOut[2], int fdStdErr[2], int fdStdIn[2], int commonfd[2])
{
    char *argv[] = {NULL};
    char *envp[] = {NULL};

    for (int fd = 0; fd != 256; ++fd)
    {
        if (fd != 0 && fd != 1 && fd != 2 && fd != fdStdOut[PIPE_WRITE] && fd != fdStdErr[PIPE_WRITE] &&
            fd != fdStdIn[PIPE_READ] && fd != commonfd[PIPE_WRITE])
            close(fd);
    }
    if (fcntl(commonfd[PIPE_WRITE], FD_CLOEXEC, 1) != 0)
    {
        BP_IGNORE(write(commonfd[PIPE_WRITE], "fcntl failure", 14));
        close(commonfd[PIPE_WRITE]);
        exit(1);
    }
    if (dup2(fdStdOut[PIPE_WRITE], 1) == -1)
        goto redirecterr;
    if (dup2(fdStdErr[PIPE_WRITE], 2) == -1)
        goto redirecterr;
    if (dup2(fdStdIn[PIPE_READ], 0) == -1)
        goto redirecterr;
#ifdef CLOSE_BEFORE_EXEC
    close(commonfd[PIPE_WRITE]);
#endif
    if (execve("/usr/bin/cat", argv, envp) == -1)
    {
#ifndef CLOSE_BEFORE_EXEC
        BP_IGNORE(write(commonfd[PIPE_WRITE], "execve failure", 15));
        close(commonfd[PIPE_WRITE]);
#endif
        exit(1);
    }
redirecterr:
    BP_IGNORE(write(commonfd[PIPE_WRITE], "Could not create one or more redirection(s)", 44));
    close(commonfd[PIPE_WRITE]);
    exit(1);
}

void ProcessMaster(int pid, int fdStdOut[2], int fdStdErr[2], int fdStdIn[2], int commonfd[2])
{
    if (commonfd[PIPE_WRITE] != -1)
        close(commonfd[PIPE_WRITE]);
    if (fdStdOut[PIPE_WRITE] != -1)
        close(fdStdOut[PIPE_WRITE]);
    if (fdStdErr[PIPE_WRITE] != -1)
        close(fdStdErr[PIPE_WRITE]);
    if (fdStdIn[PIPE_READ] != -1)
        close(fdStdIn[PIPE_READ]);

    char buf[4096];
    int len = read(commonfd[PIPE_READ], buf, 4096);

    if (len > 0)
    {
        close(commonfd[PIPE_READ]);
        return; //We have got a system error
    }
    close(commonfd[PIPE_READ]);
}


int main()
{
    int fdStdOut[2] = {-1, -1};
    int fdStdIn[2] = {-1, -1};
    int fdStdErr[2] = {-1, -1};
    int commonfd[2];

    if (pipe(commonfd) != 0)
        return (1);
    if (pipe(fdStdOut) != 0)
    {
        CleanupHandles(fdStdOut, fdStdErr, fdStdIn, commonfd);
        return (2);
    }
    if (pipe(fdStdErr) != 0)
    {
        CleanupHandles(fdStdOut, fdStdErr, fdStdIn, commonfd);
        return (3);
    }
    if (pipe(fdStdIn) != 0)
    {
        CleanupHandles(fdStdOut, fdStdErr, fdStdIn, commonfd);
        return (4);
    }
    int pid = fork();
    if (pid == -1)
    {
        CleanupHandles(fdStdOut, fdStdErr, fdStdIn, commonfd);
        return (5);
    }
    if (pid == 0)
    {
        // Worker/Child
        ProcessWorker(fdStdOut, fdStdErr, fdStdIn, commonfd);
        return (0); // This will never trigger
    }
    else // Master
    {
        int status;
        char buf[128];
        ProcessMaster(pid, fdStdOut, fdStdErr, fdStdIn, commonfd);
        write(fdStdIn[PIPE_WRITE], "test\n", 5);
        close(fdStdIn[PIPE_WRITE]);
        waitpid(pid, &status, 0);
        printf("Status %d\n", WEXITSTATUS(status));
        int res = read(fdStdOut[PIPE_READ], buf, 128);
        buf[res] = 0;
        printf("Output = %s", buf);
        return (0);
    }
}

[Index of Archives]     [Linux Assembler]     [Git]     [Kernel List]     [Fedora Development]     [Fedora Announce]     [Autoconf]     [C Programming]     [Yosemite Campsites]     [Yosemite News]     [GCC Help]

  Powered by Linux