Fix GH-8157: post_max_size evaluates .user.ini too late in php-fpm

[bukka]

This introduces new SAPI callback that runs before post read

[dzuelke]

Wanted to try it out and in the process realized that for me, modifying both post_max_size and upload_max_filesize works just fine in a .user.ini.

And then as I was fiddling with it (commenting out lines etc), I suddenly had things in a weird state where some workers were accepting the uploads and others were not.

And I realized that... maybe in all the cases where people are testing this, they're simply not seeing their config changes take effect because of user_ini.cache_ttl defaulting to 300?

[dzuelke]

(and this I think reveals a separate issue, @bukka: if .user.ini files are cached per worker process, then changes will not take effect for all workers at the same time, right?)

[bukka]

@dzuelke I just checked the test if it really fails without the patch and the test really fails (no post max exceeding error). And with the fix it works (it errors due to getting over the post max size limit). So I think the fix works and as you can see it also recreates the issue.

From what I see in the code, the point of the caching is not to parse INI config on each request which seems expected. I would have to test it to see. But do you see any issue after this patch applied?

[dzuelke]

But do you see any issue after this patch applied?

That's what I'm trying to say - it works fine for me without the patch applied, so I am wondering where the issue lies. I'll keep digging.

FWIW I am testing with a "positive" check ("does the POST data go through if the limit is raised in .user.ini"), yours is a "negative" check ("does the POST data get rejected if the limit is lowered in .user.ini"), but I don't think that's the problem here.

[bukka]

I just played with the test and again it fails without the patch and works with the test. The changes are not commited because it should metter and requires more memory (would have to extend the tester to support custom ini):

diff --git a/sapi/fpm/tests/gh8157-user-ini-post.phpt b/sapi/fpm/tests/gh8157-user-ini-post.phpt
index db291f76d24..9f66488d717 100644
--- a/sapi/fpm/tests/gh8157-user-ini-post.phpt
+++ b/sapi/fpm/tests/gh8157-user-ini-post.phpt
@@ -25,7 +25,7 @@
 EOT;
 
 $ini = <<<EOT
-post_max_size=10K
+post_max_size=80M
 html_errors=off
 EOT;
 
@@ -36,11 +36,11 @@
 $tester
     ->request(
         headers: [ 'CONTENT_TYPE' => 'application/x-www-form-urlencoded'],
-        stdin: 'foo=' . str_repeat('a', 20000),
+        stdin: 'foo=' . str_repeat('a', 20000000),
         method: 'POST',
     )
     ->expectBody([
-       'Warning: PHP Request Startup: POST Content-Length of 20004 bytes exceeds the limit of 10240 bytes in Unknown on line 0',
+       //'Warning: PHP Request Startup: POST Content-Length of 20004 bytes exceeds the limit of 10240 bytes in Unknown on line 0',
         'array(0) {',
         '}',
     ], skipHeadersCheck: true);

I should note that tester is using -n flag (no php.ini) so you might try that. Then maybe also try with minimal ini file -c to see how overwrite logic works if that's the issue.

From what I see there is an issue with not applying the .user.ini and this fixes it which I can see from the tests above.

[nielsdos]

How does this work? All implementations of pre_request_init just return SUCCESS?

[nielsdos]

Return value not checked?

[bukka]

How does this work? All implementations of pre_request_init just return SUCCESS?

Yeah looks like failing .user.ini is not terminating failure (just reflecting previous behaviour). If this should change, it should be probably through a different PR.

[bukka]

It seemed a bit strange to me too but guess that maybe it was on purpose not to fail request because .user.ini parsing fails I thought. Not sure but might potential break things if it changes so I didn't touch it.

[dzuelke]

Ahhhh, I was testing with curl -F, which ends up using multipart/form-data as the content type (like a "real" file upload would), and that is not subject to this early check.

A curl -X POST -T "somelargefile" reproduces the problem.

May I suggest adding a second test without a Content-Type, or with e.g. Content-Type: application/octet-stream, @bukka? The application/x-www-form-urlencoded case is handled explicitly in main/php_content_type.c's php_post_entries, while any other content type goes through php_default_post_reader (which then calls the same sapi_read_standard_form_data as for urlencoded form data, but that could change in the future). And maybe a third for multipart/form-data just to cover all three handler cases.

[bukka]

@dzuelke You are free to create a separate PR adding such tests but it has nothing to do with this bug fix. :) The point here is to really test that the issue got fixed which this does. Or do you see some other issue with the mentioned cases that would change after this fix?

[dzuelke]

No, it was just to guard against future regressions ;)

[arnaud-lb]

Nit:

Suggested change

	NULL /* activate_user_config */
	NULL /* pre_request_init */