> On Sept. 19, 2017, 5:01 p.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, ...)`? > > Andrew Schwartzmeyer wrote: > > 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.
> I'm not sure where Process32First came into play with this change ... I was referring to passing the 0 into `process_entry()`. But I think your reasoning makes sense. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62391/#review185709 ----------------------------------------------------------- On Sept. 19, 2017, 6 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62391/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2017, 6 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/3/ > > > 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 > >
