Skip to content

http: Decode transfer encoding first#10187

Closed
jbrobst wants to merge 1 commit intocurl:masterfrom
jbrobst:te-ce-order
Closed

http: Decode transfer encoding first#10187
jbrobst wants to merge 1 commit intocurl:masterfrom
jbrobst:te-ce-order

Conversation

@jbrobst
Copy link
Copy Markdown
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
Copy Markdown
Member

bagder commented Jan 1, 2023

@monnerat: any comments on this?

@monnerat
Copy link
Copy Markdown
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
Copy Markdown
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.

3 participants