From dca8f5841fc471866dd13401ffefe3e21ee453ea Mon Sep 17 00:00:00 2001 From: Thomas Ploch Date: Tue, 16 Dec 2014 13:23:04 +0100 Subject: [PATCH] Normalize frames for trace items since they can contain invalid data. Refs https://github.com/Seldaek/monolog/pull/474/files The fix in the previous PR did not take into account that there might be object wrapped resources that would break json_encode, so the best solution would be normalizing those frames again. @Seldaek Sorry for the inconvenience, but our graylog is still ramming up with those json_encode error messages. --- src/Monolog/Formatter/NormalizerFormatter.php | 29 +--------- .../Formatter/GelfMessageFormatterTest.php | 44 --------------- .../Formatter/NormalizerFormatterTest.php | 53 +++++++++++++++++++ 3 files changed, 55 insertions(+), 71 deletions(-) diff --git a/src/Monolog/Formatter/NormalizerFormatter.php b/src/Monolog/Formatter/NormalizerFormatter.php index 6717e40e..8a1fdab8 100644 --- a/src/Monolog/Formatter/NormalizerFormatter.php +++ b/src/Monolog/Formatter/NormalizerFormatter.php @@ -108,8 +108,8 @@ class NormalizerFormatter implements FormatterInterface if (isset($frame['file'])) { $data['trace'][] = $frame['file'].':'.$frame['line']; } else { - $frame = $this->convertResourceArgs($frame); - $data['trace'][] = json_encode($frame); + // We should again normalize the frames, because it might contain invalid items + $data['trace'][] = $this->toJson($this->normalize($frame), true); } } @@ -137,29 +137,4 @@ class NormalizerFormatter implements FormatterInterface return json_encode($data); } - - /** - * This method checks recursively for resource args inside the frame, since json_encode is choking on them. - * - * @param array $frame Reference to current frame - * @return array - */ - private function convertResourceArgs(array $frame) - { - foreach ($frame as $key => $item) { - if (is_scalar($item)) { - continue; - } - - if (is_resource($item)) { - $frame[$key] = (string) $item; - } - - if (is_array($item)) { - $frame[$key] = $this->convertResourceArgs($item); - } - } - - return $frame; - } } diff --git a/tests/Monolog/Formatter/GelfMessageFormatterTest.php b/tests/Monolog/Formatter/GelfMessageFormatterTest.php index 2cbba3fc..3f47a09a 100644 --- a/tests/Monolog/Formatter/GelfMessageFormatterTest.php +++ b/tests/Monolog/Formatter/GelfMessageFormatterTest.php @@ -182,50 +182,6 @@ class GelfMessageFormatterTest extends \PHPUnit_Framework_TestCase $this->assertEquals('pair', $message_array['_EXTkey']); } - /** - * @covers Monolog\Formatter\GelfMessageFormatter::format - */ - public function testExceptionObjectWithResourceTrace() - { - if (defined('HHVM_VERSION')) { - $this->markTestSkipped('Not supported in HHVM since it detects errors differently'); - } - - // This happens i.e. in React promises or Guzzle streams where stream wrappers are registered - // and no file or line are included in the trace because it's treated as internal function - set_error_handler(function ($errno, $errstr, $errfile, $errline ) { - throw new \ErrorException($errstr, 0, $errno, $errfile, $errline); - }); - - try { - $resource = fopen('php://memory', '+w'); - // Just do something stupid with a resource as argument - strpos($resource, 'FOO'); - } catch (\Exception $e) { - } - - // restore the error handler - restore_error_handler(); - - $formatter = new GelfMessageFormatter(); - $record = array( - 'level' => Logger::ERROR, - 'level_name' => 'ERROR', - 'channel' => 'meh', - 'context' => array('from' => 'logger', 'exception' => $e), - 'datetime' => new \DateTime("@0"), - 'extra' => array(), - 'message' => 'log' - ); - - $message = $formatter->format($record); - - $this->assertRegExp( - '%\\\\"resource\\\\":\\\\"Resource id #\d+\\\\"%', - $message->getAdditional('ctxt_exception') - ); - } - private function isLegacy() { return interface_exists('\Gelf\IMessagePublisher'); diff --git a/tests/Monolog/Formatter/NormalizerFormatterTest.php b/tests/Monolog/Formatter/NormalizerFormatterTest.php index 74778712..e5983f56 100644 --- a/tests/Monolog/Formatter/NormalizerFormatterTest.php +++ b/tests/Monolog/Formatter/NormalizerFormatterTest.php @@ -166,6 +166,41 @@ class NormalizerFormatterTest extends \PHPUnit_Framework_TestCase $this->assertEquals(@json_encode(array($resource)), $res); } + + public function testExceptionTraceWithArgs() + { + // This happens i.e. in React promises or Guzzle streams where stream wrappers are registered + // and no file or line are included in the trace because it's treated as internal function + set_error_handler(function ($errno, $errstr, $errfile, $errline) { + throw new \ErrorException($errstr, 0, $errno, $errfile, $errline); + }); + + try { + // This will contain $resource and $wrappedResource as arguments in the trace item + $resource = fopen('php://memory', 'rw+'); + fwrite($resource, 'test_resource'); + $wrappedResource = new TestStreamFoo($resource); + // Just do something stupid with a resource/wrapped resource as argument + array_keys($wrappedResource); + } catch (\Exception $e) { + restore_error_handler(); + } + + $formatter = new NormalizerFormatter(); + $record = ['context' => ['exception' => $e]]; + $result = $formatter->format($record); + + $this->assertRegExp( + '%"resource":"\[resource\]"%', + $result['context']['exception']['trace'][0] + ); + + // Tests that the wrapped resource is ignored while encoding + $this->assertRegExp( + '%\\\\"resource\\\\":null%', + $result['context']['exception']['trace'][0] + ); + } } class TestFooNorm @@ -180,3 +215,21 @@ class TestBarNorm return 'bar'; } } + +class TestStreamFoo +{ + public $foo; + public $resource; + + public function __construct($resource) + { + $this->resource = $resource; + $this->foo = 'BAR'; + } + + public function __toString() + { + fseek($this->resource, 0); + return $this->foo . ' - ' . (string) stream_get_contents($this->resource); + } +}