# Passing process handles to proc_open and co - Thread: https://externals.io/message/64173 - Original archive: https://news-web.php.net/php.internals/64173 - Started by Pierre Joye on 2012-12-06T08:49:28+00:00 - Messages: 8 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #64173 — Pierre Joye — 2012-12-06T08:49:28+00:00 URL: https://externals.io/message/64173#64173 hi! While looking at the bug #63073, I was wondering if we could simply do not pass open handles to the newly created child process. The main issue in this bug is the session related handles. They are passed to the parent process, which hangs until their are closed. It indeed does not happen (usually) until the end of the request. It makes these functions almost unusable as soon as sessions are used. These functions are about calling external commands and I fail to see which usage may require to access PHP related handles. Does anyone see any issue by not passing them anymore? And fixing this bug (and a couple of other I think)? It is windows only. Not sure if we do the same on other platforms. Cheers, -- Pierre @pierrejoye --- ## #64176 — Pierre Joye — 2012-12-06T09:52:13+00:00 URL: https://externals.io/message/64173#64176 · In reply to: #64173 I mean bug #44942. Sorry. Also to clarify for non C devs. A handle could be an opened file, a pipe or similar resources. Also important to keep in mind that users have to pass their own streams to proc_open if they like to pass or receive data. > hi! > > While looking at the bug #63073, I was wondering if we could simply do > not pass open handles to the newly created child process. > > The main issue in this bug is the session related handles. They are > passed to the parent process, which hangs until their are closed. It > indeed does not happen (usually) until the end of the request. It > makes these functions almost unusable as soon as sessions are used. > > These functions are about calling external commands and I fail to see > which usage may require to access PHP related handles. Does anyone see > any issue by not passing them anymore? And fixing this bug (and a > couple of other I think)? > > It is windows only. Not sure if we do the same on other platforms. > > Cheers, > -- > Pierre > > @pierrejoye -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #64177 — Laruence — 2012-12-06T11:02:25+00:00 URL: https://externals.io/message/64173#64177 · In reply to: #64176 > I mean bug #44942. Sorry. > > Also to clarify for non C devs. A handle could be an opened file, a > pipe or similar resources. Also important to keep in mind that users > have to pass their own streams to proc_open if they like to pass or > receive data. > Hey, but extension should has ability to change the behavior, thinking of pcntl ext thanks > > > […] > > > > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org > > -- > > > -- Laruence Xinchen Hui http://www.laruence.com/ --- ## #64178 — Laruence — 2012-12-06T11:03:38+00:00 URL: https://externals.io/message/64173#64178 · In reply to: #64177 > > > […] > Hey, > > but extension should has ability to change the behavior, thinking of pcntl ext ignore me, I mis-understood. thanks > > thanks > > […] > > > > -- > Laruence Xinchen Hui > http://www.laruence.com/ -- Laruence Xinchen Hui http://www.laruence.com/ --- ## #64179 — keisial@gmail.com — 2012-12-06T11:40:14+00:00 URL: https://externals.io/message/64173#64179 · In reply to: #64173 > hi! > > While looking at the bug #63073, I was wondering if we could simply do > not pass open handles to the newly created child process. Looking at proc_open, where there is an explicit CreateProcess (we seem to be using the library popen for the `exec()`...) If $pipes is empty and $descriptorspec doesn't require more than 0,1,2 then it should be safe not to set bInheritHandles. However, in other cases inheriting handles *will* be needed. > The main issue in this bug is the session related handles. They are > passed to the parent process, which hangs until their are closed. It > indeed does not happen (usually) until the end of the request. It > makes these functions almost unusable as soon as sessions are used. > > These functions are about calling external commands and I fail to see > which usage may require to access PHP related handles. Does anyone see > any issue by not passing them anymore? And fixing this bug (and a > couple of other I think)? > > It is windows only. Not sure if we do the same on other platforms. The issue was only detected with session files. Windows is probably the only mainstream OS without F_SETFD. When it's available, mod_files.c will set the FD_CLOEXEC, which should remove the issue from the session. The same issue could still arise from other files, though. For instance bug 44994 reported that it was happening with error.log. I don't know *why* session files are an issue. It would make sense if they were pipes and the reading was missed with the inherited child unkowingly holding a write descriptor, but these are plain files. Maybe related to the fact that they are locked, if the locks are also inherited, plus the way Windows may delay its automatic UnlockFile() after the process closes. --- ## #64180 — Pierre Joye — 2012-12-06T12:52:23+00:00 URL: https://externals.io/message/64173#64180 · In reply to: #64179 hi, > > > […] > > Looking at proc_open, where there is an explicit CreateProcess (we seem > to be using the library popen for the `exec()`...) We use CreateProcess too for our windows popen implementation, see: http://lxr.php.net/xref/PHP_5_4/TSRM/tsrm_win32.c#450 > If $pipes is empty and $descriptorspec doesn't require more than 0,1,2 > then it should be safe not to set bInheritHandles. > > However, in other cases inheriting handles *will* be needed. hm, Why? > > […] > The issue was only detected with session files. Windows is probably the > only mainstream OS without F_SETFD. When it's available, mod_files.c > will set the FD_CLOEXEC, which should remove the issue from the session. > > The same issue could still arise from other files, though. For instance > bug 44994 reported that it was happening with error.log. > > > I don't know *why* session files are an issue. It would make sense if they > were pipes and the reading was missed with the inherited child unkowingly > holding a write descriptor, but these are plain files. > Maybe related to the fact that they are locked, if the locks are also > inherited, > plus the way Windows may delay its automatic UnlockFile() after the process > closes. > > -- Pierre @pierrejoye --- ## #64184 — Pierre Joye — 2012-12-06T15:29:28+00:00 URL: https://externals.io/message/64173#64184 hi, > > > […] > Oh, right. The filename confused me into thinking it was only used in ZTS. > Yes, the popen called will be that one. > > > > […] > How else would you pass the child process the other side of a pipe ? > > (It's possible that the handles passed in STARTUPINFO hStdInput, > hStdOutput and hStdError still get automatically inherited, msdn > doesn't document that point, but it would still be relevant for higher > descriptors) Ah right, I was thinking about other handles. It seems that I may have found one possible cause, which may also cause the hang because we do not see the EOF. We should close the inherited handle after CreateProcess. And we should not always set inherit to true (as you stated earlier) when all 0-1 are given. I have to test all that in the coming days :) -- Pierre @pierrejoye --- ## #66064 — Ferenc Kovacs — 2013-02-20T18:18:02+00:00 URL: https://externals.io/message/64173#66064 · In reply to: #64184 > > Ah right, I was thinking about other handles. > > It seems that I may have found one possible cause, which may also > cause the hang because we do not see the EOF. We should close the > inherited handle after CreateProcess. And we should not always set > inherit to true (as you stated earlier) when all 0-1 are given. > > I have to test all that in the coming days :) > > Hi Pierre, did you find the time to do that? could we at least open a bug or reopen 44942 until we come up with a more robust solution? -- Ferenc Kovács @Tyr43l - http://tyrael.hu