[ErrorHandler] Don't format binary strings #53006
Conversation
|
Please note that we usually don't merge changes without a test that covers them. |
|
I can try to add a test case for that. Would loading a local, binary file into a string in a test-case be OK? (Asserting memory exhaustion in |
|
I just had a look at the tests and figured it might indeed be hard to test for |
| @@ -98,7 +98,11 @@ public function formatArgs(array $args): string | |||
| } elseif ('resource' === $item[0]) { | |||
| $formattedValue = '<em>resource</em>'; | |||
| } else { | |||
| $formattedValue = str_replace("\n", '', htmlspecialchars(var_export($item[1], true), \ENT_COMPAT | \ENT_SUBSTITUTE, $this->charset)); | |||
| if (preg_match('//u', $item[1])) { | |||
There was a problem hiding this comment.
when ppl don't use the UTF-8 charset, this might lead to false positives
what about checking using preg_match('/[^\x00\x07-\x0d\x1B\x20-\xff]/', $value) instead?
also, please unindent the if else by merging it with the surrouding one
There was a problem hiding this comment.
also, please unindent the if else by merging it with the surrouding one
The parent if checks the value of $item[0] exclusively. The merged else-if would then check $item[1]. I explicitly wanted to reduce cognitive load here (having X-branches of an if checking the same variable only to have the last one do something different is not very readable in my opinion). If you insist I'll still merge the checks though.
There was a problem hiding this comment.
If you insist I'll still merge the checks though.
We do. It's our code style.
| unset($binaryData); | ||
| $this->assertNotNull($exception, 'Could not catch RuntimeException to check for binary string replacement'); |
There was a problem hiding this comment.
Does this have to be inside a finally block?
Calling var_export on a binary string easily causes memory exhaustion if it's called with even a small image. Fixes #53005
There was a problem hiding this comment.
I applied my patch from #53005 (comment)
Good catch on the ini setting.
|
Thank you @aleho. |
FYI, originally I wanted to throw the exception to simulate the conditions where I ran into the problem. Finding out the ini setting is what made the difference I guess your changes make sense. |
Fixes #53005
This is an attempt to fix memory exhaustion when a binary string is part of a stack trace.