Tests: request body discard and error handling#31
Draft
dekobon wants to merge 3 commits intonginx:masterfrom
Draft
Tests: request body discard and error handling#31dekobon wants to merge 3 commits intonginx:masterfrom
dekobon wants to merge 3 commits intonginx:masterfrom
Conversation
The $content_length variable is going to be not available after discarding the request body. As such, the relevant location is now proxied, so the request body is not discarded. Co-Authored-By: Elijah Zupancic <e.zupancic@f5.com> Cherry-Picked-From: https://freenginx.org/hg/nginx-tests/rev/e44ee916b9598eaf805aaa9aa7afaa35db2140a5
The client_max_body_size limit should be ignored when the request body is already discarded. In HTTP/1.x, this is done by checking the r->discard_body flag when the body is being discarded, and because r->headers_in.content_length_n is 0 when it's already discarded. This, however, does not happen with HTTP/2 and HTTP/3, and therefore "error_page 413" does not work without relaxing the limit. Further, with proxy_pass, r->headers_in.content_length_n is used to determine length of the request body, and therefore is not correct if discarding of the request body isn't yet complete. While discarding the request body, r->headers_in.content_length_n contains the rest of the body to discard (or, in case of chunked request body, the rest of the current chunk to discard). Similarly, the $content_length variable uses r->headers_in.content_length if available, and also incorrect. The $content_length variable is used when proxying with fastcgi_pass, grpc_pass, and uwsgi_pass (scgi_pass uses the value calculated based on the actual request body buffers, and therefore works correctly). Co-Authored-By: Elijah Zupancic <e.zupancic@f5.com> Cherry-Picked-From: https://freenginx.org/hg/nginx-tests/rev/fe6f22da53ec760f7ab138d1d32b7a03ea7bdea3
At least h3_request_body_discard.t occasionally fails on slow hosts due to too short delays, and using longer delays fixes this. Co-Authored-By: Elijah Zupancic <e.zupancic@f5.com> Cherry-Picked-From: https://freenginx.org/hg/nginx-tests/rev/250fb78dd27079ed3cb3fd5d3b7a0132ad2ce89f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Backports test coverage from freenginx for the request body error-handling improvements in the companion nginx PR on branch
cherry/request-body-fixed-segfault-on-early-errors.These tests exercise
error_page 413handling,$content_lengthbehaviour, and proxy body forwarding when the request body has been discarded or encountered errors — across HTTP/1.x, HTTP/2, and HTTP/3. They also adjust an existing test (http_headers_multi.t) for a$content_lengthsemantic change introduced by the nginx commits.See the nginx-devel mailing list thread for the original bug report that motivated these changes.
Commits (in application order)
6647488http_headers_multi.tso$content_lengthis evaluated before body discard, since the variable is no longer available after discard.cb3152bbody_discard.t,h2_request_body_discard.t, andh3_request_body_discard.tcoveringerror_page 413,$content_length, and proxy forwarding after body discard.c60e1f1Checklist
Before creating a PR, run through this checklist and mark each as complete: