mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 62391: Fixed invalid handle bug in `os::process()`.
Date Tue, 19 Sep 2017 17:50:29 GMT


> On Sept. 19, 2017, 10:01 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp
> > Lines 550 (patched)
> > <https://reviews.apache.org/r/62391/diff/2/?file=1828686#file1828686line550>
> >
> >     So the `Process32First` iteration will actually return the 0 pid in this case?
> >     
> >     Maybe this should return `WindowsError(ERROR_INVALID_PARAMETER, ...)`?

> So the Process32First iteration will actually return the 0 pid in this case?

I'm not sure where `Process32First` came into play with this change. Could you clarify? There
are uses of `Process32First`, though not in this function; which are you referring to?

> Maybe this should return WindowsError(ERROR_INVALID_PARAMETER, ...)?

The `WindowsError` class is specifically reserved for handling errors from the Windows API
(it checks `GetLastError()`, similiar to `errno`, for you) (that is, we could allow `OpenProcess`
to fail, and then return a `WindowsError` and would expect it to contain `ERROR_INVALID_PARAMETER`).

However, since we know the input of `pid == 0` is bad, I think it is preferable to return
an `Error` directly, without invoking the OS API in a way we know will fail. Summary: this
is an `Error` and not a `WindowsError` because we have avoided causing the OS to return an
error by preventing erroneous use of the API.

That was my reasoning anyway.


> On Sept. 19, 2017, 10:01 a.m., James Peach wrote:
> > 3rdparty/stout/tests/os/process_tests.cpp
> > Lines 186 (patched)
> > <https://reviews.apache.org/r/62391/diff/2/?file=1828687#file1828687line186>
> >
> >     I think this would be easier to read if you retained a literal expected value:
> >     
> >     ```C
> >     #ifdef __WINDOWS__
> >       // ...
> >       EXPECT_EQ(0, pids.get().count(init_pid));
> >     #else
> >       EXPECT_EQ(1, pids.get().count(init_pid));
> >     #endif
> >     ```

Sure thing.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62391/#review185709
-----------------------------------------------------------


On Sept. 18, 2017, 4:39 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62391/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2017, 4:39 p.m.)
> 
> 
> Review request for mesos, James Peach and Joseph Wu.
> 
> 
> Bugs: MESOS-7988
>     https://issues.apache.org/jira/browse/MESOS-7988
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch fixes a bug found by running `mesos-tests` under Application
> Verifier on Windows. Mesos was inadvertently attempting to get a process
> handle for the System Idle Process with PID `0`, which is not permitted
> by the OS. To remedy this, we now check if `os::process` receives `0`
> for its argument, and return an error if so. Furthermore, we remove the
> PID `0` from the `os::pids` API, as it is not useful to the programmer,
> and only serves to cause errors later. Finally, the return value for
> `OpenProcess` was being incorrectly checked, as the API returns a
> `nullptr` on failure, not `INVALID_HANDLE_VALUE`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/windows/os.hpp f35bf312a25066a167dc66243ad9bf8333cb36a6

>   3rdparty/stout/tests/os/process_tests.cpp 963eb4eb3fb64d627e177d760bf42b777ae0e924

> 
> 
> Diff: https://reviews.apache.org/r/62391/diff/2/
> 
> 
> Testing
> -------
> 
> Built and ran `mesos-tests`, `libprocess-tests`, and `stout-tests` on Windows under Application
Verifier. All tests pass, and the error from `OpenProcess` is no longer being returned.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message