diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e1f9558..4f416c3 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -31,7 +31,7 @@ Metrics/CyclomaticComplexity: # Offense count: 29 # Configuration parameters: CountComments. Metrics/MethodLength: - Max: 134 + Enabled: false # Offense count: 1 # Configuration parameters: CountKeywordArgs. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e04276..761e16d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,15 @@ +## 1.1.1 + +### Bugfixes + +* frame buffer was accidentally changing encoding before header packing, which raise invalid compatible encoding errors, due to usage of "String#". this was fixed by using internal `append_str“, which does not touch encoding, and calling `String.force_encoding` in case the buffer is a mutable string passed by the user. +* dup PING frame payload passed by the user; while not really resulting in invalid encoding, the change of the input string could surprise the caller, since this would be expected to be stored somewhere so the peer PING frame can be matched on receive. + + +### Improvements + +Simplified `String#transition`, making sure it only does state machine transitions (the rest is handled outside of it). + ## 1.1.0 Several changes which improved performance for the common cases. A few highlights: diff --git a/lib/http/2/connection.rb b/lib/http/2/connection.rb index ce247a0..201ce87 100644 --- a/lib/http/2/connection.rb +++ b/lib/http/2/connection.rb @@ -403,8 +403,9 @@ def receive(data) # endpoints MAY choose to treat frames that arrive a significant # time after sending END_STREAM as a connection error. when :rst_stream - stream = @streams_recently_closed[stream_id] - connection_error(:protocol_error, msg: "sent window update on idle stream") unless stream + unless @streams_recently_closed.key?(stream_id) + connection_error(:protocol_error, msg: "sent window update on idle stream") + end else # An endpoint that receives an unexpected stream identifier # MUST respond with a connection error of type PROTOCOL_ERROR. diff --git a/lib/http/2/framer.rb b/lib/http/2/framer.rb index 7ed228f..2a36425 100644 --- a/lib/http/2/framer.rb +++ b/lib/http/2/framer.rb @@ -148,9 +148,12 @@ def common_header(frame, buffer:) header = buffer + # make sure the buffer is binary and unfrozen if buffer.frozen? header = String.new("", encoding: Encoding::BINARY, capacity: buffer.bytesize + 9) # header length - header << buffer + append_str(header, buffer) + else + header.force_encoding(Encoding::BINARY) end pack([ @@ -268,7 +271,7 @@ def generate(frame) append_str(bytes, frame[:payload]) when :ping - bytes = frame[:payload] + bytes = frame[:payload].b raise CompressionError, "Invalid payload size (#{bytes.size} != 8 bytes)" if bytes.bytesize != 8 length = 8 @@ -342,6 +345,13 @@ def generate(frame) raise CompressionError, "Invalid padding #{padlen}" end + # make sure the buffer is binary and unfrozen + if bytes.frozen? + bytes = bytes.b + else + bytes.force_encoding(Encoding::BINARY) + end + length += padlen pack([padlen -= 1], UINT8, buffer: bytes, offset: 0) frame[:flags] += [:padded] diff --git a/lib/http/2/stream.rb b/lib/http/2/stream.rb index b72ed3d..796f3b5 100644 --- a/lib/http/2/stream.rb +++ b/lib/http/2/stream.rb @@ -197,21 +197,18 @@ def calculate_content_length(data_length) # # @param frame [Hash] def send(frame) - process_priority(frame) if frame[:type] == :priority - case frame[:type] when :data - # @remote_window is maintained in send_data - send_data(frame) + # stream state management is maintained in send_data + return send_data(frame) when :window_update - manage_state(frame) do - @local_window += frame[:increment] - emit(:frame, frame) - end - else - manage_state(frame) do - emit(:frame, frame) - end + @local_window += frame[:increment] + when :priority + process_priority(frame) + end + + manage_state(frame) do + emit(:frame, frame) end end @@ -384,7 +381,7 @@ def transition(frame, sending) else event(:open) end - when :priority then process_priority(frame) + when :priority else stream_error(:protocol_error) end end @@ -405,19 +402,20 @@ def transition(frame, sending) # WINDOW_UPDATE on a stream in this state MUST be treated as a # connection error (Section 5.4.1) of type PROTOCOL_ERROR. when :reserved_local - @state = if sending - case frame[:type] - when :headers then event(:half_closed_remote) - when :rst_stream then event(:local_rst) - else stream_error - end - else - case frame[:type] - when :rst_stream then event(:remote_rst) - when :priority, :window_update then @state - else stream_error - end - end + if sending + case frame[:type] + when :headers then event(:half_closed_remote) + when :rst_stream then event(:local_rst) + when :priority + else stream_error + end + else + case frame[:type] + when :rst_stream then event(:remote_rst) + when :priority, :window_update + else stream_error + end + end # A stream in the "reserved (remote)" state has been reserved by a # remote peer. @@ -434,19 +432,20 @@ def transition(frame, sending) # PRIORITY on a stream in this state MUST be treated as a connection # error (Section 5.4.1) of type PROTOCOL_ERROR. when :reserved_remote - @state = if sending - case frame[:type] - when :rst_stream then event(:local_rst) - when :priority, :window_update then @state - else stream_error - end - else - case frame[:type] - when :headers then event(:half_closed_local) - when :rst_stream then event(:remote_rst) - else stream_error - end - end + if sending + case frame[:type] + when :rst_stream then event(:local_rst) + when :priority, :window_update + else stream_error + end + else + case frame[:type] + when :headers then event(:half_closed_local) + when :rst_stream then event(:remote_rst) + when :priority + else stream_error + end + end # A stream in the "open" state may be used by both peers to send # frames of any type. In this state, sending peers observe @@ -465,12 +464,14 @@ def transition(frame, sending) when :data, :headers, :continuation event(:half_closed_local) if end_stream?(frame) when :rst_stream then event(:local_rst) + when :priority end else case frame[:type] when :data, :headers, :continuation event(:half_closed_remote) if end_stream?(frame) when :rst_stream then event(:remote_rst) + when :priority end end @@ -490,10 +491,7 @@ def transition(frame, sending) case frame[:type] when :rst_stream event(:local_rst) - when :priority - process_priority(frame) - when :window_update - # nop here + when :priority, :window_update else stream_error end @@ -502,10 +500,7 @@ def transition(frame, sending) when :data, :headers, :continuation event(:remote_closed) if end_stream?(frame) when :rst_stream then event(:remote_rst) - when :priority - process_priority(frame) - when :window_update - # nop here + when :priority, :window_update end end @@ -534,9 +529,7 @@ def transition(frame, sending) else case frame[:type] when :rst_stream then event(:remote_rst) - when :priority - process_priority(frame) - when :window_update + when :priority, :window_update # nop else stream_error(:stream_closed) @@ -584,19 +577,15 @@ def transition(frame, sending) when :closed if sending case frame[:type] - when :rst_stream # ignore - when :priority - process_priority(frame) + when :rst_stream, :priority else stream_error(:stream_closed) unless frame[:type] == :rst_stream end - elsif frame[:type] == :priority - process_priority(frame) else case @closed when :remote_rst, :remote_closed case frame[:type] - when :rst_stream, :window_update # nop here + when :priority, :rst_stream, :window_update # nop here else stream_error(:stream_closed) end diff --git a/lib/http/2/version.rb b/lib/http/2/version.rb index 82fa53c..eda1920 100644 --- a/lib/http/2/version.rb +++ b/lib/http/2/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module HTTP2 - VERSION = "1.1.0" + VERSION = "1.1.1" end diff --git a/spec/stream_spec.rb b/spec/stream_spec.rb index fd9afb1..c30d237 100644 --- a/spec/stream_spec.rb +++ b/spec/stream_spec.rb @@ -57,7 +57,7 @@ end it "should raise error if sending invalid frames" do - frame_types.reject { |frame| %i[headers rst_stream].include?(frame[:type]) }.each do |type| + frame_types.reject { |frame| %i[headers rst_stream priority].include?(frame[:type]) }.each do |type| expect { stream.dup.send type }.to raise_error InternalError end end @@ -114,7 +114,7 @@ end it "should raise error on receipt of invalid frames" do - frame_types.reject { |frame| %i[headers rst_stream].include?(frame[:type]) }.each do |type| + frame_types.reject { |frame| %i[headers rst_stream priority].include?(frame[:type]) }.each do |type| expect { stream.dup.receive type }.to raise_error InternalError end end