Skip to content

Commit 1055a66

Browse files
stayallivestayallive
authored
Fix vsprintf not handling errors (#1855)
1 parent 77359d0 commit 1055a66

6 files changed

Lines changed: 214 additions & 7 deletions

File tree

‎src/Logs/LogsAggregator.php‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use Sentry\State\HubInterface;
1313
use Sentry\State\Scope;
1414
use Sentry\Util\Arr;
15+
use Sentry\Util\Str;
1516

1617
/**
1718
* @internal
@@ -57,7 +58,21 @@ public function add(
5758
return;
5859
}
5960

60-
$log = (new Log($timestamp, $this->getTraceId($hub), $level, vsprintf($message, $values)))
61+
$formattedMessage = Str::vsprintfOrNull($message, $values);
62+
63+
if ($formattedMessage === null) {
64+
// If formatting fails we don't format the message and log the error
65+
if ($sdkLogger !== null) {
66+
$sdkLogger->warning('Failed to format log message with values.', [
67+
'message' => $message,
68+
'values' => $values,
69+
]);
70+
}
71+
72+
$formattedMessage = $message;
73+
}
74+
75+
$log = (new Log($timestamp, $this->getTraceId($hub), $level, $formattedMessage))
6176
->setAttribute('sentry.release', $options->getRelease())
6277
->setAttribute('sentry.environment', $options->getEnvironment() ?? Event::DEFAULT_ENVIRONMENT)
6378
->setAttribute('sentry.server.address', $options->getServerName())
@@ -124,6 +139,14 @@ public function flush(): ?EventId
124139
return $hub->captureEvent($event);
125140
}
126141

142+
/**
143+
* @return Log[]
144+
*/
145+
public function all(): array
146+
{
147+
return $this->logs;
148+
}
149+
127150
private function getTraceId(HubInterface $hub): string
128151
{
129152
$span = $hub->getSpan();

‎src/Serializer/EnvelopItems/EventItem.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Sentry\Serializer\Traits\BreadcrumbSeralizerTrait;
1010
use Sentry\Serializer\Traits\StacktraceFrameSeralizerTrait;
1111
use Sentry\Util\JSON;
12+
use Sentry\Util\Str;
1213

1314
/**
1415
* @internal
@@ -124,7 +125,7 @@ public static function toEnvelopeItem(Event $event): string
124125
$payload['message'] = [
125126
'message' => $event->getMessage(),
126127
'params' => $event->getMessageParams(),
127-
'formatted' => $event->getMessageFormatted() ?? vsprintf($event->getMessage(), $event->getMessageParams()),
128+
'formatted' => $event->getMessageFormatted() ?? Str::vsprintfOrNull($event->getMessage(), $event->getMessageParams()) ?? $event->getMessage(),
128129
];
129130
}
130131
}

‎src/Util/Str.php‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sentry\Util;
6+
7+
/**
8+
* This class provides some utility methods to work with strings.
9+
*
10+
* @internal
11+
*/
12+
class Str
13+
{
14+
/**
15+
* Safe way of running `vsprintf` without throwing exceptions or errors.
16+
*
17+
* If the string could not be formatted, it returns `null`.
18+
*
19+
* @see https://www.php.net/manual/en/function.vsprintf.php
20+
*
21+
* @param array<bool|float|int|string|null> $values
22+
*/
23+
public static function vsprintfOrNull(string $message, array $values): ?string
24+
{
25+
if (empty($values)) {
26+
return $message;
27+
}
28+
29+
foreach ($values as $value) {
30+
// If the value is not a scalar or null, we cannot safely format it
31+
if (!\is_scalar($value) && $value !== null) {
32+
return null;
33+
}
34+
}
35+
36+
try {
37+
$result = @vsprintf($message, $values);
38+
39+
// @phpstan-ignore-next-line on PHP 7 `vsprintf` does not throw an exception but can return `false`
40+
return $result === false ? null : $result;
41+
} catch (\Error $e) { // This is technically a `ValueError` in PHP 8.0+ but this works in PHP 7 as well
42+
return null;
43+
}
44+
}
45+
}

‎tests/Logs/LogTest.php‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,9 @@
55
namespace Sentry\Tests\Logs;
66

77
use PHPUnit\Framework\TestCase;
8-
use Sentry\Attributes\Attribute;
98
use Sentry\Logs\Log;
109
use Sentry\Logs\LogLevel;
1110

12-
/**
13-
* @phpstan-import-type AttributeValue from Attribute
14-
* @phpstan-import-type AttributeSerialized from Attribute
15-
*/
1611
final class LogTest extends TestCase
1712
{
1813
public function testGettersAndSetters(): void

‎tests/Logs/LogsAggregatorTest.php‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sentry\Tests\Logs;
6+
7+
use PHPUnit\Framework\TestCase;
8+
use Sentry\ClientBuilder;
9+
use Sentry\Logs\LogLevel;
10+
use Sentry\Logs\LogsAggregator;
11+
use Sentry\SentrySdk;
12+
use Sentry\State\Hub;
13+
14+
final class LogsAggregatorTest extends TestCase
15+
{
16+
/**
17+
* @dataProvider messageFormattingDataProvider
18+
*/
19+
public function testMessageFormatting(string $message, array $values, string $expected): void
20+
{
21+
$client = ClientBuilder::create([
22+
'enable_logs' => true,
23+
])->getClient();
24+
25+
$hub = new Hub($client);
26+
SentrySdk::setCurrentHub($hub);
27+
28+
$aggregator = new LogsAggregator();
29+
30+
$aggregator->add(LogLevel::info(), $message, $values);
31+
32+
$logs = $aggregator->all();
33+
34+
$this->assertCount(1, $logs);
35+
36+
$log = $logs[0];
37+
38+
$this->assertEquals($expected, $log->getBody());
39+
}
40+
41+
public static function messageFormattingDataProvider(): \Generator
42+
{
43+
yield [
44+
'Simple message without values',
45+
[],
46+
'Simple message without values',
47+
];
48+
49+
yield [
50+
'Message with a value: %s',
51+
['value'],
52+
'Message with a value: value',
53+
];
54+
55+
yield [
56+
'Message with placeholders but no values: %s',
57+
[],
58+
'Message with placeholders but no values: %s',
59+
];
60+
61+
yield [
62+
'Message with placeholders but incorrect number of values: %s, %s',
63+
['value'],
64+
'Message with placeholders but incorrect number of values: %s, %s',
65+
];
66+
67+
yield [
68+
'Message with a percentage: 42%',
69+
[],
70+
'Message with a percentage: 42%',
71+
];
72+
73+
// This test case is a bit of an odd one, you would not expect this to happen in practice unless the user intended
74+
// to format the message but did not add the proper placeholder. On PHP 8+ this will return the message as is, but
75+
// on PHP 7 it will return the message without the percentage sign because some processing is done by `vsprintf`.
76+
// You would however more likely expect the previous test case where no values are provided when no placeholders are present
77+
yield [
78+
'Message with a percentage: 42%',
79+
['value'],
80+
\PHP_VERSION_ID >= 80000
81+
? 'Message with a percentage: 42%'
82+
: 'Message with a percentage: 42',
83+
];
84+
}
85+
}

‎tests/Util/StrTest.php‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Sentry\Tests\Util;
6+
7+
use PHPUnit\Framework\TestCase;
8+
use Sentry\Util\Str;
9+
10+
final class StrTest extends TestCase
11+
{
12+
/**
13+
* @dataProvider vsprintfOrNullDataProvider
14+
*/
15+
public function testVsprintfOrNull(string $message, array $values, ?string $expected): void
16+
{
17+
$this->assertSame($expected, Str::vsprintfOrNull($message, $values));
18+
}
19+
20+
public static function vsprintfOrNullDataProvider(): \Generator
21+
{
22+
yield [
23+
'Simple message without values',
24+
[],
25+
'Simple message without values',
26+
];
27+
28+
yield [
29+
'Message with a value: %s',
30+
['value'],
31+
'Message with a value: value',
32+
];
33+
34+
yield [
35+
'Message with placeholders but no values: %s',
36+
[],
37+
'Message with placeholders but no values: %s',
38+
];
39+
40+
yield [
41+
'Message with placeholders but incorrect number of values: %s, %s',
42+
['value'],
43+
null,
44+
];
45+
46+
yield [
47+
'Message with placeholder: %s',
48+
[[1, 2, 3]],
49+
null,
50+
];
51+
52+
yield [
53+
'Message with placeholder: %s',
54+
[new \stdClass()],
55+
null,
56+
];
57+
}
58+
}

0 commit comments

Comments
 (0)