Improve curl_init() return type analysis #3346
Conversation
tscni
force-pushed
the
feature/improve-curl-init-return-type
branch
from
909c3ac to
3326f37
Compare
| if ($urlArgType->isString()->yes()) { | ||
| $urlArgValue = $urlArgType->getConstantScalarValues()[0] ?? null; | ||
| if ($urlArgValue !== null) { | ||
| if (!is_string($urlArgValue)) { | ||
| throw new ShouldNotHappenException(); | ||
| } |
There was a problem hiding this comment.
This feels a bit cumbersome to do, but I couldn't find a simpler way to get the constant value of a string.
Maybe a Type::getConstantStringValue(): ?string would be useful. But then that might lead to Type exploding with helper functions eventually.
There was a problem hiding this comment.
Does Type::getConstantStrings work for you?
(And you then check all strings, not just the first)
Testcase:
$url = 'example.org';
If (rand(0,1)) $url = 'example.com';
curl_init($url);
There was a problem hiding this comment.
Ah I missed that method. But for multiple constant values it turned out to not be helpful. I overlooked that possibility completely.
Thanks a lot!
tscni
force-pushed
the
feature/improve-curl-init-return-type
branch
5 times, most recently
from
51eecd8 to
1491b5d
Compare
tscni
force-pushed
the
feature/improve-curl-init-return-type
branch
from
1491b5d to
2d47da6
Compare
| // https://github.com/php/php-src/blob/php-8.0.0/ext/curl/interface.c#L104-L107 | ||
| return new NeverType(); | ||
| } | ||
| if ($this->phpVersion->getVersionId() < 80000) { |
There was a problem hiding this comment.
Should be a new PHPVersion method
There was a problem hiding this comment.
Done (isCurloptUrlCheckingFileSchemeWithOpenBasedir())
tscni
force-pushed
the
feature/improve-curl-init-return-type
branch
from
f1498e5 to
498f2c0
Compare
|
Hi, thank you very much! This is way too much research effort for such a simple function, but I'm grateful for it. Typically I'd solve this with a benevolent union type (decreases number of false positives, does not increase the number of reported errors). (More info here: https://phpstan.org/config-reference#checkbenevolentuniontypes) |
I got annoyed by the failing e2e/php8 test (https://github.com/phpstan/phpstan/blob/1.12.x/e2e/php8/test.php#L11, https://phpstan.org/r/32a5c25f-c266-4c04-9bf1-2ec0562397d8), so I wanted to resolve it by restoring the expected issue there. (But only now do I notice that this doesn't help that at all - I completely misunderstood the issue there :P)
Following, phpstan/phpstan#1274, phpstan/phpstan#1465, phpstan/phpstan#1484 I looked into
curl_init()a bit more.Here's my analysis: (with verified behavior across all latest PHP minor versions and libcurl 7.10.5 + 8.9.1)
One important point to start with, if we presume that
curl_init()without arguments never returnsfalse, we generally ignore initialization errors or the case where curl fails to allocate more memory.curl_init()with a string argument will only return false if it fails to setCURLOPT_URLwith ithttps://github.com/php/php-src/blob/php-8.4.0beta3/ext/curl/interface.c#L1154-L1159
CURLOPT_URLwill fail with a value error on the PHP side if the URL contains a null bytehttps://github.com/php/php-src/blob/php-8.4.0beta3/ext/curl/interface.c#L139
https://github.com/php/php-src/blob/php-8.0.30/ext/curl/interface.c#L104-L107
CURLOPT_URLwill fail with a warning (falsereturn) on the PHP side if the URL contains a null bytehttps://github.com/php/php-src/blob/php-7.4.33/ext/curl/interface.c#L112-L115
But, if open_basedir is enabled, it will also fail when the URL is invalid or uses the
fileschemehttps://github.com/php/php-src/blob/php-7.4.33/ext/curl/interface.c#L146-L155
https://github.com/curl/curl/blob/curl-8_9_1/lib/setopt.c#L3256
https://github.com/curl/curl/blob/curl-8_9_1/lib/setopt.c#L1470
https://github.com/curl/curl/blob/curl-8_9_1/lib/setopt.c#L70-L71