[HttpClient] Prevent "Fatal error" in data collector #46700
Conversation
src/Symfony/Component/HttpClient/DataCollector/HttpClientDataCollector.php
Outdated
Show resolved
Hide resolved
| */ | ||
| private function argMaxLengthIsSafe(string $payload): bool | ||
| { | ||
| return strlen($payload) <= 4096; |
There was a problem hiding this comment.
4096 bytes is super small, this must be a mistake since you mention 4Mo in the comment
please also move the method at the bottom of the class and add a leading slash to \strlen
There was a problem hiding this comment.
Looking at the PHP codebase, the size seems to be in bytes.
There was a problem hiding this comment.
please also move the method at the bottom of the class and add a leading slash to \strlen
yes sir 👍, I thought php-cs-fixer would have pointed this out to me
There was a problem hiding this comment.
fabbot (aka php-cs-fixer) is on vacation I think these days, it doesn't show up in the CI reports since a few weeks :P
There was a problem hiding this comment.
Looking at the PHP codebase, the size seems to be in bytes.
OK but then we cannot go by this limit, it's way too low I think.
There was a problem hiding this comment.
On my machine, I can go up to escapeshellarg(str_repeat('-', 2097149));.
What about others?
There was a problem hiding this comment.
Well, as seen in the PHP source code, the value of cmd_max_len can be set in a bunch of different ways during compilation. So it can indeed be higher on some systems. But this is not exposed to userland.
anyway, disabling the copy as curl feature in the profile is better than doing a fatal error when running the HTTP request
There was a problem hiding this comment.
@stof because I messed up 🤦
@nicolas-grekas on my Ubuntu 22.04 (same on Debian 11)
$ getconf ARG_MAX
2097152
There was a problem hiding this comment.
So apparently on Windows we can't go above 8kio, and on linux we need to set a reasonably portable limit. Here is a suggestion, going a bit below the limit to account for escaping if that matters?
return \strlen($payload) < ('\\' === \DIRECTORY_SEPARATOR ? 8100 : 256000);
src/Symfony/Component/HttpClient/DataCollector/HttpClientDataCollector.php
Outdated
Show resolved
Hide resolved
…size of its argument is big
|
@nicolas-grekas what is missing in this PR to be merged & released ? Last 6.1 patch was released 2 days ago 😞 We are facing fatal errors in dev environment on each request using HttpClient (we use it to upload PDF files). |
|
Thank you @fmata. |
|
FYI: Unfortunately, |
Since "copy as cURL" has been added in Profiler for HttpClient usage, when payloads exceed the size allowed by escapeshellarg() we have a Fatal error 😞
The right allowed size depends on OS and is not leaked in PHP land so I prefer to be conservative and allow the lower usual value as explained in PHP source code https://github.com/php/php-src/blob/9458f5f2c8a8e3d6c65cc181747a5a75654b7c6e/ext/standard/exec.c#L77
I only added check where I think it's relevant.