[9.x] memory exhausted because JSON payload is unwanted parsed twice by Illuminate\Http\Request::createFromBase

[MircoBabin]

• Laravel Version: 9.12.2
• PHP Version: PHP 8.1.4 (cli) (built: Mar 16 2022 09:33:31) (ZTS Visual C++ 2019 x64)
• Database Driver & Version: MySql 8.0.16

Description:

I have an api POST route with json contents. Very unexpected this route was giving Allowed memory size of 134217728 bytes exhausted (tried to allocate ... bytes) errors.

To debug this issue:

1. I adjusted public/index.php so the request would run to the end and would not be aborted by memory exhausted errors.

ini_set("memory_limit", "-1");

2. I attached XDEBUG in profile mode.

3. I replayed the request.

4. I used QCacheGrind to visualize the profile:

As the profile shows, Illuminate\Http\Request::createFromBase via Request->json() calls json_decode() twice. Which is unwanted, because I don't use the Request::json() to retrieve the json contents. I do my own json_decode((string) Request::getContents()).

I did some digging in the Laravel History and found the commit introducing the json_decode behaviour in [9.x]: 13e4a7f The commit message states: ... This commit solves the underlying issue without breaking compatibility with the original functionality. ...

This commit is breaking compatibility, because it does not take memory consumption into account. Neither does it take performance into account, decoding the same request body twice is performance degradation. For large JSON (in my case Content-Length: 6428178) this can easily lead to memory exhausted errors. In Laravel 7.x there was no memory exhausted error.

[lk77]

You can remove your json_decode and use the decoded data in the request instead, with $request->all() for example, to retrieve the whole decoded data, you can also use ->get('key') to retrieve a specific field in your json, you can even use ->input('root.sub') to retrieve nested data, so there is no need to do your own json decode

If you have to do that, you should do it in your custom request (or create one with you don't use one), by overriding the ->json() method to ensure that data is not decoded twice and is consistent, but there is no reason to do it in 99% of the cases, the only exception is json_decode flags that you might need

And for the underlying issue, in my testings, json() is called once by createFromBase :

clean function does seems to do extra calls to json though

[rodrigopedra]

@IK77 Request@json has an if clause to only decode the JSON payload if it was not yet decoded.

framework/src/Illuminate/Http/Request.php

Lines 374 to 376 in 465dbee

	if (! isset($this->json)) {
	$this->json = new ParameterBag((array) json_decode($this->getContent(), true));
	}

So the subsequent calls doesn't call json_decode again.

@MircoBabin the change introduced in 13e4a7f

Replaced this call:

framework/src/Illuminate/Http/Request.php

Line 435 in 76b3417

$newRequest->request = $newRequest->getInputSource();

By this:

framework/src/Illuminate/Http/Request.php

Lines 458 to 460 in 465dbee

	if ($newRequest->isJson()) {
	$newRequest->request = $newRequest->json();
	}

So, before it called Request@getInputSource to fill the request, which in turn already decoded JSON requests on its own, since 2013, as you can see in this commit: e7f07b9

framework/src/Illuminate/Http/Request.php

Lines 390 to 397 in 465dbee

	protected function getInputSource()
	{
	if ($this->isJson()) {
	return $this->json();
	}
	return in_array($this->getRealMethod(), ['GET', 'HEAD']) ? $this->query : $this->request;
	}

Request@createFromBase has been calling Request@getInputSource, and by consequence decoding JSON requests, since 2015, introduced by this commit: 9312bec

Bottom line: I think it is very unlikely this behavior will be changed by maintainers after so many years.

You could take @IK77 suggestions and either use the already decoded data, or if really want to decode it yourself, maybe to use different parameters on json_decode, you can:

• extend the Illuminate/Http/Request class
• override its createFromBase method and avoid JSON parsing
• Modify your app's ./public/index.php to use your Request subclass
• Bind your subclass to Illuminate/Http/Request on your apps' AppServiceProvider@register method, this would look something like this:

class AppServiceProvider extends ServiceProvider
{
    public function register()
    {
        $this->app->bind(\Illuminate\Http\Request::class, \App\Http\Request::class);
    }

    public function boot()
    {
    }
}

Note, while FormRequests will still extend the original Request class and not your subclass, they are hydrated from the request resolved from the container, which after binding it to the container, will be an instance of your subclass.

framework/src/Illuminate/Foundation/Providers/FormRequestServiceProvider.php

Lines 33 to 37 in 465dbee

	$this->app->resolving(FormRequest::class, function ($request, $app) {
	$request = FormRequest::createFrom($app['request'], $request);
	$request->setContainer($app)->setRedirector($app->make(Redirector::class));
	});

Hope this helps you solve your issue.

[rodrigopedra]

@driesvints after doing research to answer @MircoBabin , I noted PR #37921 , which introduced the check if a request is a JSON request before replacing its $request property, apparently didn't change anything as Request@getInputSource already did that check internally.

Actually the tests (one changed, and one added) on that PR, both passes if one reverts the change made by it.

I am not sure it makes any change if reverted, as for non-JSON requests Request@getInputSource would return the contents of $request->request anyways which are already populated on the Request@duplicate call some lines above, but it is curious tests passed without the proposed change.

I have no idea on how to do so, but maybe there is a way to automate to check if any added tests fail before committing a PR's proposed change?

[driesvints]

Thanks @rodrigopedra.

@LukeTowers could you weigh in here?

[MircoBabin]

I have taken @IK77 advice and do not json_decode() myself anymore. Instead i'm using Request::json() now.

But still I'm getting memory exhausted because Laravel is decoding twice. Here is new the profile:

[MircoBabin]

@rodrigopedra thanks for you detailed explanation. So it seems I'm wrong concluding 13e4a7f was the commit introducing wrong behaviour.

Your solution to extend the Illuminate/Http/Request, while being a very creative solution, is not really a viable solution. Upon each Laravel update I still would have to compare the Request::createFromBase with the extended class.

Walking this path it would be easier to just directly modify the Illuminate/Http/Request.php source file, what would break with each Laravel update. I could write a test for this, that would fail after update. But I'm not going to take this path, as it is not maintainable in the long run. (Yep against all best practices, I am committing the vendor directory. Because on shared webhosting you can't composer install, there is no shell. Using DeployHQ ftp deployment with a committed vendor directory is the easiest solution.)

[driesvints]

To be fair, isn't the real issue that your JSON payloads are simply too large? What kind of payloads are you handling?

[MircoBabin]

The payloads are order request payloads for renting a touringcar. In the payload also is the calculated Google Maps route present. That together constitutes a lot of bytes.

The http-payload is ~7 MB, which is not that big. Json_decoded() it becomes ~69MB, which still is not beyond default PHP limits.

But then comes Laravel, deciding to decode twice and keeping 2 copies in memory. So claiming 2 times 69MB which is beyond the default PHP limits.

[taylorotwell]

If you have a solution to this issue that does not break other use cases please PR it. Thank you.

[LukeTowers]

after doing research to answer @MircoBabin , I noted PR #37921 , which introduced the check if a request is a JSON request before replacing its $request property, apparently didn't change anything as Request@getInputSource already did that check internally.

I am not sure it makes any change if reverted, as for non-JSON requests Request@getInputSource would return the contents of $request->request anyways which are already populated on the Request@duplicate call some lines above, but it is curious tests passed without the proposed change.

@rodrigopedra thanks for your detailed response! My change however is absolutely critical, although it might not appear so at first glance which is why the original behaviour was so dangerous.

The problem is that the internal request property is intended to only hold POST parameters; while Request@getInputSource will return GET parameters (the query property) if the request method is GET or HEAD. This means that any calls to Laravel's methods asking for POST data on a GET request would return GET data previously (i.e. Request::post('somevar'); would return the value of ?somevar=bad on GET requests). The Request@duplicate call above does not have this problem because the request and query parameters are properly separated during the duplication process.

Ultimately there are two things to keep in mind:

1. The Request->request property should ONLY ever hold POST data
2. The Request@getInputSource() method returns data from different locations depending on the type of request made.

[LukeTowers]

@MircoBabin where is Laravel calling json_decode twice? I'm not seeing it in the source.