Buffer encoding issues

.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.

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:

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.

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]

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

lib/http/2/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module HTTP2
-  VERSION = "1.1.0"
+  VERSION = "1.1.1"
 end

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