Skip to content

Fix unchecked request-body flatten errors in apache2handler - #25176

Open
joshtrichards wants to merge 1 commit into
php:masterfrom
joshtrichards:jtr/fix-postAPR2
Open

joshtrichards wants to merge 1 commit into
php:masterfrom
joshtrichards:jtr/fix-postAPR2

Conversation

@joshtrichards

Copy link
Copy Markdown

php_apache_sapi_read_post() checks ap_get_brigade() errors but ignores the return value of apr_brigade_flatten().

If a bucket read fails during flattening, APR returns before updating the supplied length. The callback then counts the requested length as successfully read, potentially reporting bytes that were not written into the destination buffer.

Apache's core input filter normally reads socket buckets and propagates errors through ap_get_brigade(), so ordinary network failures are already handled. This change defensively handles any bucket-read failure occurring during the subsequent flatten operation; no such failure has been demonstrated with the standard input-filter chain.

Check the flatten result after cleaning up the brigade and return zero on failure, matching the existing handling of ap_get_brigade() errors. Successful reads are unchanged.

Related:

php_apache_sapi_read_post() checks ap_get_brigade() errors but ignores the return value of apr_brigade_flatten().

If a bucket read fails during flattening, APR returns before updating the supplied length. The callback then counts the requested length as successfully read, potentially reporting bytes that were not written into the destination buffer.

Apache's core input filter normally reads socket buckets and propagates errors through ap_get_brigade(), so ordinary network failures are already handled. This change defensively handles any bucket-read failure occurring during the subsequent flatten operation; no such failure has been demonstrated with the standard input-filter chain.

Check the flatten result after cleaning up the brigade and return zero on failure, matching the existing handling of ap_get_brigade() errors. Successful reads are unchanged.

Assisted-by: Copilot:gpt-6.1-sol

@devnexen devnexen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in practice, seems it require very specific conditions to happen, master is the appropriate branch indeed IMHO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants