Poco process terminate and pid reuse

[marlowabnp]

I think I have found a problem in the way that Poco terminates processes. The issue is in the Process class. The launch method returns a process handle which can be used to wait for process completion. So far, so good. However, the kill method takes a pid, not a process handle. This is asymmetric and causes a problem that I will describe below:

If a poco program launches a process which is then killed by someone else and then another process is started and the pid gets reused and then poco tries to kill the process it launched, poco will try to kill the wrong process. This is a serious problem. I presume there is a similar issue in the requestTermination method which also takes a pid. In my opinion pids should not be used for termination due to pid reuse.

The issue can be mitigated on UNIX which has proper process management. If anyone else kills the poco child, the parent will receive SIGCHLD so it can update its records. It will then know that the pid it has is now out of date. This is still extra work it has to do to guard against pid reuse but at least it can be done. Not so on Windoze. On Windoze the parent and child do not have a close association, no SIGCHLD or equivalent gets delivered to the parent so there is no way that the parent can know that the pid it has for its child is still valid.

On Windoze one can keep a process handle open for a child that the parent has launched. In Process_WIN32.cpp in the killImpl method a fresh process handle is opened given the pid. I think it would be better if Poco used the process handle that was returned from the launch method. I know this will involve interface changes but I reckon the interface itself is broken since it is not reliable to terminate by pid on Windoze.

I see from the code that ProcessHandle encapsulates the pid. This is good. I reckon this means the change won't be too difficult. This should fix the problem for Windoze. UNIX can still kill by pid, in fact, it has to, so that still leaves the issue open for UNIX. For completeness Poco may have to handle SIGCHLD and invalidate the ProcessHandle. I am not sure about how this would work exactly.

I welcome comments on these issues and design thoughts.

This is not a merely theoretical problem. I have worked on a general purpose program launcher that kept track of its processes using pids and it sometimes went wrong because of this.

Regards,

Andrew Marlow

[marlowabnp]

After some thought I think I know what the solution is. Process::kill should take a process handle. On UNIX it can use the process handle to get the pid and then kill by pid. On windoze it uses the WIN32 process handle directly, the one that was returned from the launch method.

Killing by pid on Unix will work in this case provided that the app that uses Poco::Process::launch does so in a dedicated thread. It can wait for the process to finish which it will if it is ever killed by someone/something else. The problem of pid reuse only applies if the app uses the pid to see (at regular intervals) if the process it launched is still alive. Therefore, what I said earlier about SIGCHLD does not apply. Pid reuse is only a problem for apps that do not wait for the child they spawned (in theory there is a small timing window between the spawn and the wait but I don't think there's anything that can be done about that and the window is tiny).

In fact, IMO the process should be launched detached in order to avoid the zombie problem. I realise that some apps will want the attached launch that Poco does at the moment so I think this needs to be a choice the user of the API can make. This is what ACE does, FWIW.

So in conclusion, can Poco be changed? Process::kill needs to kill by ProcessHandle and Process::launch needs to be able to specify that the process is detached.

Regards,

Andrew Marlow

[marlowabnp]

I really do need launch to be able to specify that the process is detached. Please please can it be added? It solves all sorts of issues. For a start, it means that on UNIX you do not have to worry about zombies if the launcher is killed. Second, it means a launched process has I/O that is independent. On Windoze I had a launched program that asked for input and what happened was that the launcher asked for the input. I was launching a batch file that did a pause. The pause prompt and input read occurred in the parent launcher.

Regards,

Andrew Marlow

[marlowabnp]

After some thought I think I know what the solution is. Process::kill should take a process handle. On UNIX it can use the process handle to get the pid and then kill by pid. On windoze it uses the WIN32 process handle directly, the one that was returned from the launch method.

Killing by pid on Unix will work in this case provided that the app that uses Poco::Process::launch does so in a dedicated thread. It can wait for the process to finish which it will if it is ever killed by someone/something else. The problem of pid reuse only applies if the app uses the pid to see (at regular intervals) if the process it launched is still alive. Therefore, what I said earlier about SIGCHLD does not apply. Pid reuse is only a problem for apps that do not wait for the child they spawned (in theory there is a small timing window between the spawn and the wait but I don't think there's anything that can be done about that and the window is tiny).

I would really appreciate an answer to this question please. It involves a change to the public interface so I cannot just submit a patch.

Perhaps an overloaded kill method should be added. This would at least preserve the existing public interface. The old interface would continue to be vunerable to the pid reuse problem. What do people think? If I submitted such a patch what is the chance it would be accepted?

[guenter]

I'm in favor of an overloaded kill() method in the Process class. As for launching detached processes, IMO the best way would be to add an option argument (int) to the second launch() method, with a default of 0. We can then define an enumeration, allowing to specify different options to launch.

Code: Select all

class Process: ...
{
public:
    enum LaunchOptions
    {
        PROC_LAUNCH_DETACHED = 0x01
    };
    ...
    static ProcessHandle launch(const std::string& command, const Args& args, Pipe* inPipe, Pipe* outPipe, Pipe* errPipe, int options = 0);
    ...
};

It would be great if you could implement this.

[marlowabnp]

I'm in favor of an overloaded kill() method in the Process class.

It would be great if you could implement this.

I have sent a patch for this to Gunter. I wanted to be able to attach it to this post but I cannot see how to do that using this sort of forum.

I haven't got around to the enhancement for detatched processes yet but I hope to get around to it soon.

[guenter]

I've added the patch to 1.4.3