Fix header table size assignment

lib/http/2/connection.rb

@@ -667,12 +667,15 @@ def connection_settings(frame)
           end
 
         when :settings_header_table_size
-          # Setting header table size might cause some headers evicted
+          # The value is the maximum table size for the encoder of the other
+          # side: our setting limits the peer's encoder (our decompressor),
+          # and the peer's setting limits our encoder (our compressor).
+          # The table itself changes with the next dynamic table size update.
           case side
           when :local
-            @compressor.table_size = v
-          when :remote
             @decompressor.table_size = v
+          when :remote
+            @compressor.table_size = v
           end
 
         when :settings_enable_push

lib/http/2/header/compressor.rb

@@ -11,9 +11,11 @@ def initialize(settings = Settings.new)
         @cc = EncodingContext.new(settings)
       end
 
-      # Set dynamic table +size+ in EncodingContext
+      # Set the maximum dynamic table +size+ allowed by the peer's
+      # SETTINGS_HEADER_TABLE_SIZE. The next header block starts with a
+      # dynamic table size update when the table size has to change.
       def table_size=(size)
-        @cc.table_size = size
+        @cc.max_table_size = size
       end
 
       # Encodes +i+ via integer representation into +buffer+ at the offset set by +offset+.

lib/http/2/header/decompressor.rb

@@ -15,9 +15,11 @@ def initialize(settings = Settings.new)
         @cc = EncodingContext.new(settings)
       end
 
-      # Set dynamic table +size+ in EncodingContext
+      # Set the maximum dynamic table +size+ allowed by our acknowledged
+      # SETTINGS_HEADER_TABLE_SIZE. When the current table is larger, the
+      # next header block has to start with a dynamic table size update.
       def table_size=(size)
-        @cc.table_size = size
+        @cc.max_table_size = size
       end
 
       # Decodes integer value from provided +buffer+.

lib/http/2/header/encoding_context.rb

@@ -101,12 +101,20 @@ class EncodingContext
 
       # Initializes compression context with appropriate client/server
       # +settings+ and maximum size of the dynamic table.
+      #
+      # The dynamic table starts at +settings.table_size+ octets. The same
+      # value is the initial maximum size allowed for the table.
       def initialize(settings = Settings.new)
         @table = []
         @table_by_field = Hash.new { |hs, k| hs[k] = [] }
         @unshifts = 0
         @settings = settings
+        # Current dynamic table size, as known to both encoder and decoder.
         @limit = settings.table_size
+        # Maximum dynamic table size allowed by SETTINGS_HEADER_TABLE_SIZE.
+        @max_limit = @limit
+        # Smallest maximum size set since the last dynamic table size update.
+        @lowest_max_limit = @limit
         @_table_updated = false
         @current_table_size = 0
       end
@@ -118,11 +126,15 @@ def dup
         tbf = @table_by_field.transform_values(&:dup)
         unshifts = @unshifts
         l = @limit
+        ml = @max_limit
+        lml = @lowest_max_limit
         other.instance_eval do
           @table = t.dup # shallow copy
           @table_by_field = tbf
           @unshifts = unshifts
           @limit = l
+          @max_limit = ml
+          @lowest_max_limit = lml
         end
         other
       end
@@ -152,16 +164,21 @@ def process(cmd)
         name = cmd[:name]
         value = cmd[:value]
 
+        # The maximum size was reduced below the current table size and the
+        # encoder has not signalled a size that fits into it.
+        raise CompressionError, "dynamic table size update required" if type != :changetablesize && @lowest_max_limit < @limit
+
         case type
         when :changetablesize
           raise CompressionError, "tried to change table size after adding elements to table" if @_table_updated
 
-          # we can receive multiple table size change commands inside a header frame. However,
-          # we should blow up if we receive another frame where the new table size is bigger.
-          table_size_updated = @limit != @settings.table_size
-
-          raise CompressionError, "dynamic table size update exceed limit" if !table_size_updated && value > @limit
+          # The new maximum size MUST be lower than or equal to the limit set
+          # by SETTINGS_HEADER_TABLE_SIZE.
+          # - https://www.rfc-editor.org/rfc/rfc7541#section-6.3
+          raise CompressionError, "dynamic table size update exceed limit" if value > @max_limit
 
+          # The smallest maximum size set since the last update is signalled.
+          @lowest_max_limit = @max_limit if value <= @lowest_max_limit
           self.table_size = value
 
           nil
@@ -210,10 +227,26 @@ def process(cmd)
       end
 
       # Plan +headers+ compression.
+      #
+      # Emits dynamic table size updates first when the table size has to
+      # change. See #max_table_size=.
       def encode(headers)
         # Literals commands are marked with :noindex when index is not used
         noindex = STATIC_NEVER.include?(@settings.index)
 
+        if @lowest_max_limit < @limit
+          self.table_size = @lowest_max_limit
+          yield({ type: :changetablesize, value: @limit })
+        end
+        @lowest_max_limit = @max_limit
+
+        size = @settings.table_size
+        size = @max_limit if @max_limit < size
+        if size != @limit
+          self.table_size = size
+          yield({ type: :changetablesize, value: size })
+        end
+
         headers.each do |field, value|
           # Literal header names MUST be translated to lowercase before
           # encoding and transmission.
@@ -276,6 +309,24 @@ def table_size=(size)
         resize_table(0)
       end
 
+      # Set the maximum dynamic table +size+ allowed by
+      # SETTINGS_HEADER_TABLE_SIZE.
+      #
+      # The current table size does not change here. It changes only with a
+      # dynamic table size update at the beginning of a header block:
+      # - an encoder emits the update in the next #encode call. It uses
+      #   +settings.table_size+, capped by this maximum.
+      # - a decoder requires the update in the next header block when the
+      #   current table size exceeds this maximum.
+      #
+      # When the maximum is reduced and then increased again before the next
+      # header block, the smallest maximum has to be signalled first.
+      # - https://www.rfc-editor.org/rfc/rfc7541#section-4.2
+      def max_table_size=(size)
+        @max_limit = size
+        @lowest_max_limit = size if size < @lowest_max_limit
+      end
+
       def listen_on_table
         yield
       ensure

sig/header/encoding_context.rbs

@@ -26,6 +26,10 @@ module HTTP2
 
       @limit: Integer
 
+      @max_limit: Integer
+
+      @lowest_max_limit: Integer
+
       @_table_updated: bool
 
       def dup: () -> EncodingContext
@@ -40,6 +44,8 @@ module HTTP2
 
       def table_size=: (Integer) -> void
 
+      def max_table_size=: (Integer) -> void
+
       def listen_on_table: { () -> void } -> void
 
       private

spec/client_spec.rb

@@ -90,6 +90,88 @@
     end
   end
 
+  context "header table size" do
+    def sent_headers(conn)
+      frames = []
+      conn.on(:frame) { |bytes| frames << f.parse(bytes) }
+      yield
+      frames.select { |frame| frame[:type] == :headers }
+    end
+
+    def peer_settings(payload)
+      settings = settings_frame
+      settings[:payload] = payload
+      f.generate(settings)
+    end
+
+    def response_headers(stream, payload)
+      f.generate(type: :headers, stream: stream.id, flags: END_HEADERS | END_STREAM, payload: payload)
+    end
+
+    it "should signal remote SETTINGS_HEADER_TABLE_SIZE reduction in the next header block" do
+      client << peer_settings([[:settings_header_table_size, 0]])
+
+      headers = sent_headers(client) do
+        client.new_stream.headers(REQUEST_HEADERS, end_stream: true)
+        client.new_stream.headers(REQUEST_HEADERS, end_stream: true)
+      end
+
+      # Dynamic table size update to 0.
+      expect(headers[0][:payload].getbyte(0)).to eq 0x20
+      expect(headers[1][:payload].getbyte(0)).not_to eq 0x20
+
+      peer = Decompressor.new
+      peer.table_size = 0
+      headers.each { |frame| expect(peer.decode(frame[:payload])).to eq REQUEST_HEADERS }
+    end
+
+    it "should keep own table size when remote SETTINGS_HEADER_TABLE_SIZE is larger" do
+      client << peer_settings([[:settings_header_table_size, 65_536], [:settings_max_concurrent_streams, 1000]])
+
+      # The peer's decoder keeps the default table size until it receives
+      # a dynamic table size update.
+      peer = Decompressor.new
+
+      # Unique headers fill the table, a repeated one refers to an old entry.
+      extra = Array.new(100) { |i| [["x-request-id", format("%<i>03d-%<pad>s", i: i, pad: "a" * 40)], %w[x-app shared]] }
+      headers = sent_headers(client) do
+        extra.each { |h| client.new_stream.headers(REQUEST_HEADERS + h, end_stream: true) }
+      end
+
+      headers.zip(extra).each do |frame, h|
+        expect(peer.decode(frame[:payload]).last(2)).to eq h
+      end
+    end
+
+    it "should accept header blocks with a table size update after local SETTINGS_HEADER_TABLE_SIZE reduction" do
+      client = Client.new(settings_header_table_size: 256)
+      client << peer_settings([])
+      stream = client.new_stream
+      stream.headers(REQUEST_HEADERS, end_stream: true)
+      client << f.generate(type: :settings, stream: 0, payload: [], flags: ACK)
+      expect(client.local_settings[:settings_header_table_size]).to eq 256
+
+      peer = Compressor.new
+      peer.table_size = 256
+      received = nil
+      stream.on(:headers) { |h| received = h }
+      client << response_headers(stream, peer.encode(RESPONSE_HEADERS + [%w[x-app shared]]))
+
+      expect(received).to eq RESPONSE_HEADERS + [%w[x-app shared]]
+    end
+
+    it "should reject header blocks without a table size update after local SETTINGS_HEADER_TABLE_SIZE reduction" do
+      client = Client.new(settings_header_table_size: 256)
+      client << peer_settings([])
+      stream = client.new_stream
+      stream.headers(REQUEST_HEADERS, end_stream: true)
+      client << f.generate(type: :settings, stream: 0, payload: [], flags: ACK)
+
+      payload = Compressor.new.encode(RESPONSE_HEADERS)
+      expect { client << response_headers(stream, payload) }.to raise_error(CompressionError)
+    end
+  end
+
   context "upgrade" do
     it "fails when client has already created streams" do
       client.new_stream

spec/compressor_spec.rb

@@ -237,6 +237,101 @@
 
         expect { cc.process(type: :changetablesize, value: 150_000_000) }.to raise_error(CompressionError)
       end
+
+      it "should accept table size update up to increased maximum" do
+        cc = EncodingContext.new(Settings.new(table_size: 4096))
+        cc.max_table_size = 8192
+
+        expect { cc.process(type: :changetablesize, value: 8192) }.not_to raise_error
+        expect { cc.process(type: :changetablesize, value: 8193) }.to raise_error(CompressionError)
+      end
+
+      it "should not change table size when maximum is reduced" do
+        cc = EncodingContext.new(Settings.new(table_size: 2048))
+        cc.process(name: "test1", value: "1" * 1024, type: :incremental)
+        cc.max_table_size = 0
+
+        expect(cc.table.size).to eq 1
+      end
+
+      it "should require table size update when maximum is reduced below table size" do
+        cc = EncodingContext.new
+        cc.max_table_size = 256
+
+        expect { cc.process(name: 1, type: :indexed) }.to raise_error(CompressionError)
+      end
+
+      it "should require smallest maximum when maximum is reduced and increased" do
+        cc = EncodingContext.new
+        cc.max_table_size = 256
+        cc.max_table_size = 4096
+
+        cc.process(type: :changetablesize, value: 1024)
+        expect { cc.process(name: 1, type: :indexed) }.to raise_error(CompressionError)
+
+        cc = EncodingContext.new
+        cc.max_table_size = 256
+        cc.max_table_size = 4096
+
+        cc.process(type: :changetablesize, value: 256)
+        cc.process(type: :changetablesize, value: 4096)
+        expect { cc.process(name: 1, type: :indexed) }.not_to raise_error
+      end
+    end
+
+    context "table size updates" do
+      def updates(buffer)
+        cmds = []
+        dc = Decompressor.new
+        until buffer.empty?
+          cmd = dc.header(buffer)
+          break unless cmd[:type] == :changetablesize
+
+          cmds << cmd[:value]
+        end
+        cmds
+      end
+
+      it "should not emit table size update without changes" do
+        expect(updates(c.encode(REQUEST_HEADERS))).to be_empty
+      end
+
+      it "should emit table size update in the next header block after maximum is reduced" do
+        c.table_size = 256
+
+        expect(updates(c.encode(REQUEST_HEADERS))).to eq [256]
+        expect(updates(c.encode(REQUEST_HEADERS))).to be_empty
+      end
+
+      it "should emit smallest and final table size when maximum is reduced and increased" do
+        c.table_size = 256
+        c.table_size = 1024
+
+        expect(updates(c.encode(REQUEST_HEADERS))).to eq [256, 1024]
+      end
+
+      it "should emit smallest table size when maximum is reduced and restored" do
+        c.table_size = 256
+        c.table_size = 4096
+
+        expect(updates(c.encode(REQUEST_HEADERS))).to eq [256, 4096]
+      end
+
+      it "should not grow table above own table size" do
+        c.table_size = 65_536
+
+        expect(updates(c.encode(REQUEST_HEADERS))).to be_empty
+      end
+
+      it "should keep decoder in sync" do
+        d.table_size = 0
+        c.table_size = 0
+
+        3.times do
+          expect(d.decode(c.encode(REQUEST_HEADERS))).to eq REQUEST_HEADERS
+        end
+        expect(d.instance_eval { @cc.table }).to be_empty
+      end
     end
 
     context "encode" do