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

Fix possible crash in the OOM bootstrapper #442

Merged
merged 3 commits into from
Apr 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ jobs:
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php-version }}
coverage: none
imjoehaines marked this conversation as resolved.
Show resolved Hide resolved

- run: composer validate

Expand Down
1 change: 1 addition & 0 deletions .github/workflows/unstable-version-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ jobs:
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php-version }}
coverage: none

- run: composer validate

Expand Down
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
Changelog
=========

## TBD

### Bug Fixes

* Fix a possible crash in the OOM bootstrapper with an incomplete container
[#442](https://github.com/bugsnag/bugsnag-laravel/pull/442)

## 2.22.0 (2021-02-10)

### Enhancements
Expand Down
1 change: 1 addition & 0 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
<testsuites>
<testsuite name="Bugsnag Laravel Test Suite">
<directory suffix="Test.php">./tests</directory>
<directory suffix=".phpt">./tests/phpt</directory>
</testsuite>
</testsuites>

Expand Down
25 changes: 18 additions & 7 deletions src/OomBootstrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,26 @@ public function bootstrap()

$lastError = error_get_last();

// If this is an OOM and memory increase is enabled, bump the memory
// limit so we can report it
if ($lastError !== null
&& app('bugsnag')->getMemoryLimitIncrease() !== null
&& preg_match($this->oomRegex, $lastError['message'], $matches) === 1
) {
if (!$lastError) {
return;
}

$isOom = preg_match($this->oomRegex, $lastError['message'], $matches) === 1;

if (!$isOom) {
return;
}

/** @var \Bugsnag\Client|null $client */
$client = app('bugsnag');

// If the client exists and memory increase is enabled, bump the
// memory limit so we can report it. The client can be missing when
// the container isn't complete, e.g. when unit tests are running
if ($client && $client->getMemoryLimitIncrease() !== null) {
$currentMemoryLimit = (int) $matches[1];

ini_set('memory_limit', $currentMemoryLimit + app('bugsnag')->getMemoryLimitIncrease());
ini_set('memory_limit', $currentMemoryLimit + $client->getMemoryLimitIncrease());
}
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
--TEST--
The OomBootstrapper should not crash when bugsnag is not in the DI container
--FILE--
<?php

use Bugsnag\BugsnagLaravel\OomBootstrapper;

function app($alias = null) {
echo "'app' was called!\n";

if ($alias === 'bugsnag') {
return null;
}

if ($alias !== null) {
throw new UnexpectedValueException("Unknown alias '{$alias}' given");
}

throw new BadFunctionCallException("This fake 'app' should always be called with an alias");
}

require __DIR__.'/../../src/OomBootstrapper.php';

(new OomBootstrapper())->bootstrap();

ini_set('memory_limit', '5M');

$i = 0;

gc_disable();

while ($i++ < 12345678) {
$a = new stdClass;
$a->b = $a;
}

echo "No OOM!\n";
?>
--SKIPIF--
<?php
if (PHP_MAJOR_VERSION < 7) {
echo "SKIP - PHP 5 does not run OOM in this test";
}
?>
--EXPECTF--
Fatal error: Allowed memory size of %d bytes exhausted (tried to allocate %d bytes) in %s on line %d
'app' was called!
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
--TEST--
The OomBootstrapper should not crash when bugsnag is in the DI container
--FILE--
<?php

use Bugsnag\BugsnagLaravel\OomBootstrapper;

class FakeClient
{
public function getMemoryLimitIncrease()
{
echo "'getMemoryLimitIncrease' was called!\n";

return 12345;
}
}

function app($alias = null) {
echo "'app' was called!\n";

if ($alias === 'bugsnag') {
return new FakeClient();
}

if ($alias !== null) {
throw new UnexpectedValueException("Unknown alias '{$alias}' given");
}

throw new BadFunctionCallException("This fake 'app' should always be called with an alias");
}

require __DIR__.'/../../src/OomBootstrapper.php';

(new OomBootstrapper())->bootstrap();

ini_set('memory_limit', '5M');

$i = 0;

gc_disable();

while ($i++ < 12345678) {
$a = new stdClass;
$a->b = $a;
}

echo "No OOM!\n";
?>
--SKIPIF--
<?php
if (PHP_MAJOR_VERSION < 7) {
echo "SKIP - PHP 5 does not run OOM in this test";
}
?>
--EXPECTF--
Fatal error: Allowed memory size of %d bytes exhausted (tried to allocate %d bytes) in %s on line %d
'app' was called!
'getMemoryLimitIncrease' was called!
'getMemoryLimitIncrease' was called!