Fix chunked ending check #1607
Merged
Fix chunked ending check #1607
+50
−0
Conversation
This file contains 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
* master: test_pumactl.rb - remove skip on test_control_url binder.rb - move logger.log calls after adding listeners appveyor.yml - fix casing, OpenSSL 1.1.0h, convert to ps v3.12.0 Update url for binder parsing test for JRuby
|
We're running into same problem. This branch seems to fix it for us. Is this PR being considerd for master, @schneems ? |
|
Great PR! Thanks! What's a good way to test this with another tool? Does curl allow me to send chunked requests? What would a good terminal command be for me to test this out? |
|
Curl will automatically send chunked requests by setting the header (link): For example
Outputs: Debugging this request in a receiving puma server in Unfortunately I couldn't find any info on how to set the chunk size in order to send multiple chunks. Requests larger than 16380 bytes seem to automatically get chunked, however debugging these large requests in Puma shows wrong results. I suspect these large requests are written to a Tempfile to limit memory usage or something. |
|
@schneems Found a way to do it with require 'uri'
require 'net/http'
require 'tempfile'
class Chunked
def initialize(data, chunk_size)
@size = chunk_size
if data.respond_to? :read
@file = data
end
end
def read(_, offset)
if @file
@file.read(@size, offset)
end
end
def eof!
@file.eof!
end
def eof?
@file.eof?
end
end
uri = URI::parse('http://example.test')
connection = Net::HTTP.new(uri.host, uri.port)
connection.set_debug_output($stdout)
file = Tempfile.new
file.write("Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod
tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam,
quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse
cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non
proident, sunt in culpa qui officia deserunt mollit anim id est laborum.")
file.rewind
chunked = Chunked.new(file, 100)
request = Net::HTTP::Post.new uri.request_uri, {'Transfer-Encoding' => 'chunked', 'content-type' => 'text/plain'}
request.body_stream = chunked
connection.start do |http|
http.request(request)
end
file.close
file.unlinkOutputs (response isn't relevant): |
|
This will also fix #1492 |
|
Awesome! thanks for the working repro case. I confirm this fails without your patch and passes with it. Thanks a ton! |
|
Hi @schneems can you say when the next version will be released? |
|
I don't honestly know. I can try to queue up a relase. In the mean time can you use the |
|
Reminder - appveyor creates & saves a pre-compiled gem file. As I recall, the test files are saved in the gem, along with a windows specific rake file. It's installed and the tests run from it. It's built for ruby 2.2. thru 2.6.. Given the date, I'm sure trunk won't have any breaking API or ABI changes before 2.6 release. Be the first on your block to release a 2.6 compatible pre-compiled gem! |
nateberkopec
pushed a commit
that referenced
this pull request
Aug 3, 2019
…request (#1812) * Fix a bug that the last CRLF of chunked body may be used in the next request The last CRLF of chunked body is checked by #1607. But it's incomplete. If a client sends the last CRLF (or just LF) after Puma processes "0\r\n" line, the last CRLF (or just LF) isn't dropped in the "0\r\n" process: https://github.com/puma/puma/blob/675344e8609509b0d767ae7680436b3b382d8394/lib/puma/client.rb#L183-L192 if line.end_with?("\r\n") len = line.strip.to_i(16) if len == 0 @body.rewind rest = io.read # rest is "" with no the last CRLF case and # "\r" with no last LF case. # rest.start_with?("\r\n") returns false for # Both of these cases. rest = rest[2..-1] if rest.start_with?("\r\n") @buffer = rest.empty? ? nil : rest set_ready return true end The unprocessed last CRLF (or LF) is used as the first data in the next request. Because Puma::Client#reset sets `@parsed_bytes` to 0. https://github.com/puma/puma/blob/675344e8609509b0d767ae7680436b3b382d8394/lib/puma/client.rb#L100-L109 def reset(fast_check=true) @parsed_bytes = 0 It means that data in `@buffer` (it's "\r" in no the last LF case) and unread data in input socket (it's "\r\n" in no the last CRLF case and "\n" in no the last LF case) are used used as the first data in the next request. This change fixes these cases by the followings: * Ensures reading the last CRLF by setting `@partial_part_left` when CRLF isn't read in processing "0\r\n" line. * Introduces a `@in_last_chunk` new state to detect whether the last CRLF is waiting or not. It's reset in Puma::Client#reset. * Remove unnecessary returns #1812 (comment) is the location where this rule is made. * Add missing last CRLF for chunked request in tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As stated in #1480 according to the spec a valid chunked body is formatted like this:
An example would be something like this:
Notice the 2 CLRF's at the ending of the request. One is of the last last-chunk and the other one is the trailing CRLF of the chunked-body. Currently it Puma only accounts for a single CLRF and incorrectly sees the second CLRF as the start of a following request, which is malformed.
This PR checks for a second CLRF and skips it if present.
Fixes #1456
Fixes #1480
As a sidenode, does Puma parse trailer-parts?