Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetExitCodeProcess should not be used to check if process is active #2289

Open
MieszkoP opened this issue Oct 7, 2024 · 1 comment
Open

Comments

@MieszkoP
Copy link

MieszkoP commented Oct 7, 2024

Some code compiled in the Win32 platform use the GetExitCodeProcess function and its lpExitCode out parameter in a way that may not be correct. According to the documentation, STILL_ACTIVE should not be used to determine whether a process is terminated because GetExitCodeProcess works correctly only if process is already terminated.

I found incorrect uses of GetExitCodeProcess in several files:
ACE.cpp

int
ACE::process_active (pid_t pid)
{
#if !defined(ACE_WIN32)
....
#else
  // Create a handle for the given process id.
  ACE_HANDLE process_handle =
    ::OpenProcess (PROCESS_QUERY_INFORMATION, FALSE, pid);
  if (process_handle == ACE_INVALID_HANDLE || process_handle == 0)
    return 0;
  else
    {
      DWORD status;
      int result = 1;
      if (::GetExitCodeProcess (process_handle,
                                &status) == 0
          || status != STILL_ACTIVE)
        result = 0;

      ::CloseHandle (process_handle);
      return result;
    }
#endif /* !ACE_WIN32 */
}

Instead, it should probably be something like this

if (::WaitForSingleObject(process_handle, 0) == WAIT_TIMEOUT)
          result = 1;
      else if (::WaitForSingleObject(process_handle, 0) == WAIT_OBJECT_0)
          result = 0;
      else
          result = -1;

in place of

 if (::GetExitCodeProcess (process_handle,
                                &status) == 0
          || status != STILL_ACTIVE)
        result = 0;

In file ace/Process.cpp occurs similar problem:

DWORD code;
BOOL result = ::GetExitCodeProcess (this->gethandle (), &code);
return result && code == STILL_ACTIVE;

should be changed to:

BOOL result = (WaitForSingleObject(this->gethandle(), 0) == WAIT_TIMEOUT);
return result;

function ACE_Process_Manager::handle_signal () should be accordingly changed. Below is my suggestion for improvement

int
ACE_Process_Manager::handle_signal (int,
                                    siginfo_t *si,
                                    ucontext_t *)
{
#if defined (ACE_WIN32)
  ACE_HANDLE proc = si->si_handle_;
  ACE_exitcode status = 0;
  status = WaitForSingleObject(proc, 0);
  if (status == WAIT_OBJECT_0) // 
  {
          ....
  }
  else if (status == WAIT_TIMEOUT)
    ACELIB_ERROR_RETURN ((LM_ERROR,
                           ACE_TEXT ("Process still active")
                           ACE_TEXT (" -- shouldn't have been called yet!\n")),
                          0); // return 0 : stay registered
  else
  {
      // <WaitForSingleObject> failed.
      ACELIB_ERROR_RETURN ((LM_ERROR,
                         ACE_TEXT ("WaitForSingleObject failed\n")),
                        -1); // return -1: unregister
  }
....
}
@mitza-oci
Copy link
Member

Please create a pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants