[issue36067] subprocess terminate() "invalid handle" error when process is gone

[Eryk Sun]

Eryk Sun <eryksun at gmail.com> added the comment:

The subprocess Handle object is never finalized explicitly, so the Process handle should always be valid as long as we have a reference to the Popen instance. We can call TerminateProcess as many times as we want, and the handle will still be valid. If it's already terminated, NtTerminateProcess fails with STATUS_PROCESS_IS_TERMINATING, which maps to Windows ERROR_ACCESS_DENIED. 

If some other code mistakenly called CloseHandle on our handle. This is a serious bug that should never be silenced and always raise an exception if we can detect it. 

If the handle value hasn't been reused, NtTerminateProcess fails with STATUS_INVALID_HANDLE. If it now references a non-Process object, it fails with STATUS_OBJECT_TYPE_MISMATCH. Both of these map to Windows ERROR_INVALID_HANDLE. If the handle value was reused by a Process object (either via CreateProcess[AsUser] or OpenProcess) that lacks the PROCESS_TERMINATE (1) right (cannot be our original handle, since ours had all access), then it fails with STATUS_ACCESS_DENIED, which maps to Windows ERROR_ACCESS_DENIED. Otherwise if it has the PROCESS_TERMINATE right, then currently we'll end up terminating an unrelated process. As mentioned by Giampaolo, we could improve our chances of catching this bug by first verifying the PID via GetProcessId and the creation time from GetProcessTimes. We'd also have to store the creation time in _execute_child. Both functions would have to be added to _winapi.

> The solution I propose just ignores ERROR_INVALID_HANDLE and 
> makes it an alias for "process is already gone". 

If we get ERROR_INVALID_HANDLE, we should not try to call GetExitCodeProcess. All we know is that it either wasn't a valid handle or was reused to reference a non-Process object. Maybe by the time we call GetExitCodeProcess it has since been reused again to reference a Process. That would silence the error and propagate a bug by setting an unrelated exit status. Otherwise, GetExitCodeProcess will just fail again with ERROR_INVALID_HANDLE. There's no point to this, and it's potentially making the problem worse. 

---

Also, unrelated but something I noticed. Using _active list in Windows shouldn't be necessary. Unlike Unix, a process in Windows doesn't have to be waited on by its parent to avoid a zombie. Keeping the handle open will actually create a zombie until the next _cleanup() call, which may be never if Popen() isn't called again.

----------
nosy: +eryksun

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue36067>
_______________________________________