Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: Decode transfer encoding first #10187

Closed
wants to merge 1 commit into from
Closed

http: Decode transfer encoding first #10187

wants to merge 1 commit into from

Conversation

jbrobst
Copy link
Contributor

@jbrobst jbrobst commented Dec 31, 2022

The unencoding stack is added to as Transfer-Encoding and Content-Encoding fields are encountered with no distinction between the two, meaning the stack will be incorrect if, e.g., the message has both fields and a non-chunked Transfer-Encoding comes first. This PR fixes this by ordering the stack with transfer encodings first.

Additionally, something I did not change but I think may be a bug is that MAX_ENCODE_STACK doesn't limit writer_stack's length, just the number of encodings in a single field line.

@bagder
Copy link
Member

bagder commented Jan 1, 2023

@monnerat: any comments on this?

@monnerat
Copy link
Contributor

monnerat commented Jan 1, 2023

Yes, this makes sense. Without it we depend on the ordering of headers within the received stream.
The special case of chunked encoding still remains though as it is handled externally (always first). I guess it should be taken as a known limitation, unless we move it to the regular unencoding stack, which is a big work.

@bagder bagder added the HTTP label Jan 1, 2023
@bagder bagder closed this in aa6e7a1 Jan 1, 2023
@bagder
Copy link
Member

bagder commented Jan 1, 2023

Thanks!

bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
The unencoding stack is added to as Transfer-Encoding and
Content-Encoding fields are encountered with no distinction between the
two, meaning the stack will be incorrect if, e.g., the message has both
fields and a non-chunked Transfer-Encoding comes first. This commit
fixes this by ordering the stack with transfer encodings first.

Reviewed-by: Patrick Monnerat
Closes curl#10187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants