At 08:02 PM 5/23/2003, Jeff Trawick wrote:
OWWW! That's a case I hadn't considered. However, the right answer is to close the children's side of the pipes, always.
apr_proc_create() does that unconditionally in the parent path after fork()
it should be changed to register a cleanup-for-exec on the parent's side of the pipes... I suspect that this particular hole can be filled immediately before mutexing and related API compatibility concerns are worked out, unless somebody can come up with what sort of app this would break
If they are left open, it's much harder to detect that the child has died/you've reached EOF.
For mod_ext_filter I can hold a thread mutex around the critical section above,
>and that protects it from other instances of mod_ext_filter doing fork() >and inheriting the wrong handle, but it does nothing for other threads >calling apr_proc_create().
The right answer, in part, is to close those child handles.
here I'm confused because it looks like apr_proc_create() already does that
The second side is to protect that window where we declare the handles as keep-open on exec to the point where we've already fork()ed.
agreed
It would seem that all of the pipe creation and child cleanup
>registration needs to be done inside apr_proc_create() so that >it can hold a mutex and all callers of apr_proc_create() will >be happy. I didn't look at it to know if that would require >API changes. I wouldn't be shocked.
For the small window between declaring the handles keep-open, fork()ing the child and then closing them in the parent, we absolutely need a mutex to keep things clean.
yes; basically, get a mutex at start of apr_proc_create, then create pipes as noted in apr_procattr_io_set(), then fork() then close child side of handles and register cleanup-on-exec for other handles,
then release mutex
so apr_procattr_io_set() is changed to note what to do later in apr_proc_create() instead of actually creating the pipes
all the while, keep track of which if any function is permanently lost this way and also which changes require potential adjustment in the app :)
