fix the return status check of subprocess

[fouzhe]

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.

[SR4ven]

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?

boofuzz/boofuzz/utils/debugger_thread_simple.py

Lines 155 to 156 in d47fe79

	exit_info = os.waitpid(self.pid, 0)
	self.exit_status = exit_info[1] # [0] is the pid

[fouzhe]

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?

boofuzz/boofuzz/utils/debugger_thread_simple.py

Lines 155 to 156 in d47fe79

	exit_info = os.waitpid(self.pid, 0)
	self.exit_status = exit_info[1] # [0] is the pid

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".

boofuzz/boofuzz/utils/debugger_thread_simple.py

Lines 170 to 171 in d47fe79

	else:
	reason = default_reason

boofuzz/boofuzz/utils/debugger_thread_simple.py

Lines 183 to 185 in d47fe79

	msg = "[{0}] Crash. Exit code: {1}. Reason - {2}\n".format(
	time.strftime("%I:%M.%S"), self.exit_status if self.exit_status is not None else "<unknown>", reason
	)

[SR4ven]

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?

[fouzhe]

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.

[SR4ven]

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.

[SR4ven]

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?

[fouzhe]

@SR4ven Sorry for the delay.
Yeah, I think an exit with code 0 should not be picked up as a test case failure.

[SR4ven]

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?

[jtpereyda]

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.