jtpereyda
fix the return status check of subprocess
The description of os.waitpid is:
waitpid(...)
waitpid(pid, options) -> (pid, status)
Wait for completion of a given child process.
The return value of os.waitpid is a tuple thus the self.exit_status can not be None.
self.exit_status is set to the second element of the tuple so why can't it be None? Is the status always guaranteed to be set?
https://github.com/jtpereyda/boofuzz/blob/d47fe7905013d3f215f36f73f9fd6e06dee39ee5/boofuzz/utils/debugger_thread_simple.py#L155-L156
self.exit_statusis set to the second element of the tuple so why can't it be None? Is the status always guaranteed to be set?https://github.com/jtpereyda/boofuzz/blob/d47fe7905013d3f215f36f73f9fd6e06dee39ee5/boofuzz/utils/debugger_thread_simple.py#L155-L156
The second element of the return value of os.waitpid can't be None (as refer to the source code).
Hence, when some process exits with status 0 (which means it completes successfully), the msg save to self.process_monitor.last_synopsis will also contain "xxx Crash. Exit code: xxx".
https://github.com/jtpereyda/boofuzz/blob/d47fe7905013d3f215f36f73f9fd6e06dee39ee5/boofuzz/utils/debugger_thread_simple.py#L170-L171
https://github.com/jtpereyda/boofuzz/blob/d47fe7905013d3f215f36f73f9fd6e06dee39ee5/boofuzz/utils/debugger_thread_simple.py#L183-L185
Ok so with this PR a process exiting with status 0 won't be logged as a crash anymore, will it.
Makes sense to me, but will the process monitor automatically start the target process again for the next test case?
It depends on the logic of the process monitor. I didn't modify any logic of the process monitor. I just modified the log content when the target process exits.
True. Currently the logging and error handling are closely interwoven.
So the process monitor will detect a failure whenever msg is not empty.
As far as I can tell a process exiting with status 0 will not generate an error message so the process monitor won't detect a failure. So far so good, that's how it's supposed to be. The only possible problem I see here is that the process won't be restarted by the process monitor because there was no failure. I'll check if that's the case, then we're good to merge.
Sorry for the delay, I just ran some tests with and without the changes from this PR.
I'm using a target process that immediately exits with code 0. When I start a fuzzing session on the current master branch, a failure will be detected after the first test case with the following log message:
Check Failed: ProcessMonitor#140290317399904[127.0.0.1:26002] detected crash on test case #1: [11:11.20] Crash. Exit code: 0. Reason - Exit with code - 0
The procmon will then restart the target process as expected.
Running the same test with the changes from this PR, I get the same behaviour. A failure is detected and the target gets restarted. The only difference is that the log message is less verbose. It's missing the info about the exit code, which I think is really helpful, even if it is code 0 for success.
Check Failed: ProcessMonitor#139924430083936[127.0.0.1:26002] detected crash on test case #1:
Was this fix intended to change the procmon behaviour so that an exit with code 0 is no longer picked up as a test case failure by boofuzz?
@SR4ven Sorry for the delay. Yeah, I think an exit with code 0 should not be picked up as a test case failure.
Ok, thanks for clearing that up @fouzhe.
Correct me if I'm wrong, but from my testing this PR doesn't seem to implement this behaviour. The only thing that changed is the verbosity of the log output. In case of the exit code being 0, that information is no longer appended to the log message.
Do you intend to implement the behaviour change?
Yeah, I think an exit with code 0 should not be picked up as a test case failure.
A typical use case for boofuzz is to target a persistent server, in which case exiting with 0 may be a failure of sorts, indicating a potential DoS. However, this will depend on the use case. We should probably support exit code 0 as a non-failure, but still requiring a restart.
