DOCGroup / ACE_TAO

ACE and TAO
https://www.dre.vanderbilt.edu/~schmidt/TAO.html
701 stars 378 forks source link

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

Open MieszkoP opened 1 week ago

MieszkoP commented 1 week ago

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 commented 1 week ago

Please create a pull request