Bug fixed #61471

[axot]

This is an improved bug fix for #61471 related to #2180.

[krakjoe]

@weltling please can you deal with this one, since you are familiar with previous patch ;)

[weltling]

Thanks for the continuation, @axot.

@krakjoe well, i'm indeed faminial with reverting the commit :) What would be great is that the reporters of the issue in the previous ticket could retest the new patch, @alexpott ping. On my side, I also had compilation issues with the old patch and APache 2.4.10 somehow, need to retest that as well.

Thanks.

[krakjoe]

Okay, I'll deal with it when we it has been reviewed :)

[axot]

@krakjoe @weltling Thank you for handling this PR. Hope this time all drupal test cases can be passed.

[nikic]

I think @laruence handled this last time :)

[krakjoe]

Thanks @nikic, reassigned ...

[krakjoe]

ping @laruence can we get your input here please ?

[alexpott]

@axot I've built PHP7 including your PR and the tests that were failing now pass. That's not to say there might be some other effect but at least the original issue we had won't happen again.

[axot]

@alexpott Thanks for testing this!

[staabm]

@alexpott coud you provide your testcode so we can add it with this PR?

[alexpott]

@staabm I was running parts of the Drupal 8 test suite so I'm not sure that that is relevant.

[nikic]

LGTM, only question I have is whether post_read_data should really be in the main SAPI globals rather than the Apache specific ones?

[axot]

@nikic Hi, I moved post_read_error from SAPI globals to AP2, is it seems better now?

[krakjoe]

@weltling are SAPI (specific) globals part of ABI, they would appear to be ?

[weltling]

@krakjoe indeed, any exported structs are part of ABI. The change is acceptable in this case, was no ABI breach in the previous variant, too. Otheriwse, as it only one file related, the flag could fit to be a thread local.

I've also rebuilt this with an older Apache 2.4, seems fine now.

Thanks.

[krakjoe]

ofc, end of struct ...

[weltling]

Actually doesn't matter where, even on 32-bit with default 4-byte alignment it'll still have a gap of one byte.

Thanks.

[nikic]

Merged via 80c8d84. Thanks!

[vobruba-martin]

@weltling It looks like that partially uploaded upload_tmp_dir/php* files aren't deleted after this patch and our temp directory is growing in size because of this.

[axot]

Thank you for reporting. call php_apache_request_dtor should fix this?

> git diff
diff --git i/sapi/apache2handler/sapi_apache2.c w/sapi/apache2handler/sapi_apache2.c
index 7fc81bb4e0..55daca9c76 100644
--- i/sapi/apache2handler/sapi_apache2.c
+++ w/sapi/apache2handler/sapi_apache2.c
@@ -719,6 +719,7 @@ zend_first_try {

        if (AP2(post_read_error)) {
                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "Error while attempting to read POST data: %d", SG(sapi_headers).http_response_code);
+               php_apache_request_dtor(r);
                apr_brigade_cleanup(brigade);
                PHPAP_INI_OFF;
                return SG(sapi_headers).http_response_code;

[nikic]

Looks like 7.0.18 was just branched. I think 80c8d84 should be reverted for now and the reverts included in the upcoming 7.0.18/7.1.4 releases. @weltling @krakjoe

[vobruba-martin]

I think this should be reverted ASAP because it opens door for an easy DoS attack. See my bug report https://bugs.php.net/bug.php?id=74318

[weltling]

Taking in account also 80c8d84#commitcomment-21472893, we're going to indeed revert this patch again :(

@axot Appreciation to your work! Looks like we need more QA for this fix. The recommendation is to target 7.2 with a followup PR, and try to gather more feedback. When the known issues are fixed, we'll have a good chance in the 7.2 pre phase.

Thanks.