famzah / popen-noshell

A much faster popen() and system() implementation for Linux
68 stars 13 forks source link

hang in pclose_noshell() if multiple pipes are open for writing #7

Closed dkondor closed 9 years ago

dkondor commented 9 years ago

Hi,

I just found this library, it is very useful for me (my main problem with the standard popen() implementation was that if I disable memory overcommit, it fails for a program with large memory usage). However, I found the following issue: if I open multiple pipes for writing (the use-case is writing compressed output with '/usr/bin/zip output.zip -'), the plose_noshell() function can hang (i.e. not return, top shows the program consuming 0% CPU, this occurs in the waipit() call at line 540). In relation, the child processes do not terminate (do not reach the 'zombie' state). Note that I use the zip program, which reads from its stdin until it receives EOF.

My guess for the cause is that the processes started later also inherit the write end of the pipes of previously started processes, resulting in that the fclose() call at line 536 does not really close the pipe as other handles also reference it in the processes which were started later. In accordance with this, adding the FD_CLOEXEC flag to the write end of the pipes created in popen_noshell() after starting the child process fixes this problem for me. This can be achieved with the following patch (applied to popen_noshell.c):

351,352d350 < int flags = fcntl(pipefd[0],F_GETFD); < fcntl(pipefd[0],F_SETFD,flags | FD_CLOEXEC); 356,357d353 < int flags = fcntl(pipefd[1],F_GETFD); < fcntl(pipefd[1],F_SETFD,flags | FD_CLOEXEC);

On the other hand, I'm not an expert on dealing with file descriptors (that's why I try to avoid using fork / vfork in the first place), so it can be something more complicated. Also, there might still be a problem if an other thread calls fork() or clone() between creating the pipe and setting this flag.

I will try to attach a minimal test-case if I can.

Best regards, Daniel

dkondor commented 9 years ago

I could not figure out how to attach a source file here, I've uploaded a minimal test case to reproduce this here: http://kdani88.web.elte.hu/popen2.c

famzah commented 9 years ago

Hello. Your analysis is absolutely correct and helped me a lot in debugging this. Thank you; also for the minimal test-case.

I've fixed the issue by setting the close-on-exec flag when the pipe file descriptors are created. This is atomic and avoids race-conditions.

As a side note, are you using the library in a multi-threaded environment?

dkondor commented 9 years ago

Hi, thanks, your commit fixed the issue for me. I'm not using multiple threads, just multiple output files from one thread; I remembered having a somewhat similar issue some time ago in another program when I was using pipe() / fork() / exec() from a multithreaded program. I'm not sure if I was aware of pipe2(), although I must have seen it as it has a shared manpage with pipe() on my system :) Best regards, Daniel