Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail if unserialize is unable to parse output of child process #230

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MarcHagen
Copy link

@MarcHagen MarcHagen commented Aug 28, 2024

In ParallelProcess in getOutput() is checked if the unserialize is successful.
When it fails, is sets the errorOutput to the process output.

$this->output = @unserialize(base64_decode($processOutput));
if (! $this->output) {
$this->errorOutput = $processOutput;
}

But this is not validated anymore because the check is already passed.

public function triggerSuccess()
{
if ($this->getErrorOutput()) {
$this->triggerError();
return;
}
$output = $this->getOutput();
foreach ($this->successCallbacks as $callback) {
call_user_func_array($callback, [$output]);
}
return $output;
}

Moving the getOutput() before the error output check will ensure all error output cases are handled properly.

Adding extra checks on the output of the child processes, we can ensure the output of the task is not false or null, falsely triggering the errorOutput.
By changing the ChildRuntime output to a predictable array ensures we can check if something is wrong with parsing the child processes output.

@MarcHagen MarcHagen changed the title Fail if unserialize is unable to parse output Fail if unserialize is unable to parse output of child process Aug 28, 2024
MarcHagen and others added 4 commits August 29, 2024 13:37
In `ParallelProcess` in `getOutput()` is checked if the `unserialize` is successful. 
When it fails, is sets the `errorOutput` to the process output. 
But this is not validated anymore because the check is already passed.
Moving the `getOutput()` before the error output check will ensure all error output cases are handled properly.
Any writes to `php://stdout` will break the unserialization of child process results
But it did because the child process output was "childTjs=" but this is incorrect and cannot be unserialized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant