THRIFT-6126: Handle unknown Compact and JSON types

lib/rb/ext/compact_protocol.c

@@ -92,10 +92,11 @@ static int get_compact_type(VALUE type_value) {
   } else if (type == TTYPE_UUID) {
     return CTYPE_UUID;
   } else {
-    char str[50];
-    sprintf(str, "don't know what type: %d", type);
-    rb_raise(rb_eStandardError, "%s", str);
-    return 0;
+    rb_exc_raise(get_protocol_exception(
+      INT2FIX(PROTOERR_INVALID_DATA),
+      rb_sprintf("Unknown compact type: %d", type)
+    ));
+    return 0; /* unreachable */
   }
 }
 
@@ -416,10 +417,11 @@ static int8_t get_ttype(int8_t ctype) {
   } else if (ctype == CTYPE_UUID) {
     return TTYPE_UUID;
   } else {
-    char str[50];
-    sprintf(str, "don't know what type: %d", ctype);
-    rb_raise(rb_eStandardError, "%s", str);
-    return 0;
+    rb_exc_raise(get_protocol_exception(
+      INT2FIX(PROTOERR_INVALID_DATA),
+      rb_sprintf("Unknown compact type: %d", ctype)
+    ));
+    return 0; /* unreachable */
   }
 }
 

lib/rb/lib/thrift/protocol/compact_protocol.rb

@@ -89,14 +89,22 @@ def self.is_bool_type?(b)
 
       def self.get_ttype(compact_type)
         val = COMPACT_TO_TTYPE[compact_type & 0x0f]
-        raise "don't know what type: #{compact_type & 0x0f}" unless val
-        val
+        return val if val
+
+        raise ProtocolException.new(
+          ProtocolException::INVALID_DATA,
+          "Unknown compact type: #{compact_type & 0x0f}"
+        )
       end
 
       def self.get_compact_type(ttype)
         val = TTYPE_TO_COMPACT[ttype]
-        raise "don't know what type: #{ttype & 0x0f}" unless val
-        val
+        return val if val
+
+        raise ProtocolException.new(
+          ProtocolException::INVALID_DATA,
+          "Unknown compact type: #{ttype}"
+        )
       end
     end
 

lib/rb/lib/thrift/protocol/json_protocol.rb

@@ -183,42 +183,27 @@ def get_type_name_for_type_id(id)
       when Types::UUID
         "uid"
       else
-        raise NotImplementedError
+        raise ProtocolException.new(ProtocolException::INVALID_DATA, "Unknown type id: #{id}")
       end
     end
 
     def get_type_id_for_type_name(name)
-      if (name == "tf")
-        result = Types::BOOL
-      elsif (name == "i8")
-        result = Types::BYTE
-      elsif (name == "i16")
-        result = Types::I16
-      elsif (name == "i32")
-        result = Types::I32
-      elsif (name == "i64")
-        result = Types::I64
-      elsif (name == "dbl")
-        result = Types::DOUBLE
-      elsif (name == "str")
-        result = Types::STRING
-      elsif (name == "rec")
-        result = Types::STRUCT
-      elsif (name == "map")
-        result = Types::MAP
-      elsif (name == "set")
-        result = Types::SET
-      elsif (name == "lst")
-        result = Types::LIST
-      elsif (name == "uid")
-        result = Types::UUID
+      case name
+      when "tf" then Types::BOOL
+      when "i8" then Types::BYTE
+      when "i16" then Types::I16
+      when "i32" then Types::I32
+      when "i64" then Types::I64
+      when "dbl" then Types::DOUBLE
+      when "str" then Types::STRING
+      when "rec" then Types::STRUCT
+      when "map" then Types::MAP
+      when "set" then Types::SET
+      when "lst" then Types::LIST
+      when "uid" then Types::UUID
       else
-        result = Types::STOP
+        raise ProtocolException.new(ProtocolException::INVALID_DATA, "Unknown type name: #{name.inspect}")
       end
-      if (result == Types::STOP)
-        raise NotImplementedError
-      end
-      return result
     end
 
     # Static helper functions

lib/rb/spec/compact_protocol_spec.rb

@@ -136,6 +136,35 @@
     end
   end
 
+  it "should reject unknown field and container types as invalid protocol data" do
+    {
+      :read_field_begin => [0x1e],
+      :read_list_begin => [0x1e]
+    }.each do |reader_method, bytes|
+      trans = Thrift::MemoryBufferTransport.new(bytes.pack("C*"))
+      proto = Thrift::CompactProtocol.new(trans)
+
+      expect { proto.public_send(reader_method) }.to raise_error(Thrift::ProtocolException, "Unknown compact type: 14") do |error|
+        expect(error.type).to eq(Thrift::ProtocolException::INVALID_DATA)
+      end
+    end
+  end
+
+  it "should report the original unknown type when writing fields and containers" do
+    {
+      :write_field_begin => [nil, 99, 1],
+      :write_list_begin => [99, 1]
+    }.each do |writer_method, args|
+      trans = Thrift::MemoryBufferTransport.new
+      proto = Thrift::CompactProtocol.new(trans)
+
+      expect { proto.public_send(writer_method, *args) }.to raise_error(Thrift::ProtocolException, "Unknown compact type: 99") do |error|
+        expect(error.type).to eq(Thrift::ProtocolException::INVALID_DATA)
+      end
+      expect(trans.available).to eq(0)
+    end
+  end
+
   it "should decode i32 minima from direct canonical zigzag bytes" do
     trans = Thrift::MemoryBufferTransport.new
     trans.write(INTEGER_MINIMUM_ENCODINGS[:i32].pack("C*"))

lib/rb/spec/json_protocol_spec.rb

@@ -283,8 +283,8 @@
     end
 
     it "should get type name for type id" do
-      expect { @prot.get_type_name_for_type_id(Thrift::Types::STOP) }.to raise_error(NotImplementedError)
-      expect { @prot.get_type_name_for_type_id(Thrift::Types::VOID) }.to raise_error(NotImplementedError)
+      expect { @prot.get_type_name_for_type_id(Thrift::Types::STOP) }.to raise_error(Thrift::ProtocolException, "Unknown type id: 0")
+      expect { @prot.get_type_name_for_type_id(Thrift::Types::VOID) }.to raise_error(Thrift::ProtocolException, "Unknown type id: 1")
       expect(@prot.get_type_name_for_type_id(Thrift::Types::BOOL)).to eq("tf")
       expect(@prot.get_type_name_for_type_id(Thrift::Types::BYTE)).to eq("i8")
       expect(@prot.get_type_name_for_type_id(Thrift::Types::DOUBLE)).to eq("dbl")
@@ -299,7 +299,7 @@
     end
 
     it "should get type id for type name" do
-      expect { @prot.get_type_id_for_type_name("pp") }.to raise_error(NotImplementedError)
+      expect { @prot.get_type_id_for_type_name("pp") }.to raise_error(Thrift::ProtocolException, 'Unknown type name: "pp"')
       expect(@prot.get_type_id_for_type_name("tf")).to eq(Thrift::Types::BOOL)
       expect(@prot.get_type_id_for_type_name("i8")).to eq(Thrift::Types::BYTE)
       expect(@prot.get_type_id_for_type_name("dbl")).to eq(Thrift::Types::DOUBLE)
@@ -313,6 +313,20 @@
       expect(@prot.get_type_id_for_type_name("lst")).to eq(Thrift::Types::LIST)
     end
 
+    it "should reject unknown field and container types as invalid protocol data" do
+      @trans.write('{"1":{"wat":0}}')
+      @prot.read_struct_begin
+      expect { @prot.read_field_begin }.to raise_error(Thrift::ProtocolException, 'Unknown type name: "wat"') do |error|
+        expect(error.type).to eq(Thrift::ProtocolException::INVALID_DATA)
+      end
+
+      trans = Thrift::MemoryBufferTransport.new('["wat",0]')
+      prot = Thrift::JsonProtocol.new(trans)
+      expect { prot.read_list_begin }.to raise_error(Thrift::ProtocolException, 'Unknown type name: "wat"') do |error|
+        expect(error.type).to eq(Thrift::ProtocolException::INVALID_DATA)
+      end
+    end
+
     it "should read json syntax char" do
       @trans.write('F')
       expect { @prot.read_json_syntax_char('G') }.to raise_error(Thrift::ProtocolException)

lib/rb/spec/server_spec.rb

@@ -19,6 +19,7 @@
 #
 require 'spec_helper'
 require 'openssl'
+require 'timeout'
 
 describe 'Server' do
   describe Thrift::BaseServer do
@@ -49,6 +50,31 @@
   end
 
   describe Thrift::SimpleServer do
+    class EphemeralServerSocket < Thrift::ServerSocket
+      def initialize(ready)
+        super('127.0.0.1', 0)
+        @ready = ready
+      end
+
+      def listen
+        super
+        @ready << handle.addr[1]
+      end
+    end
+
+    class StopAfterVoidHandler
+      attr_reader :calls
+
+      def initialize
+        @calls = 0
+      end
+
+      def voidMethod
+        @calls += 1
+        throw :stop
+      end
+    end
+
     before(:each) do
       @processor = double("Processor")
       @serverTrans = double("ServerTransport")
@@ -106,6 +132,62 @@
       expect(@serverTrans).to receive(:close).ordered
       expect { @server.serve }.to throw_symbol(:stop)
     end
+
+    {
+      Thrift::CompactProtocolFactory.new => proc do
+        trans = Thrift::MemoryBufferTransport.new
+        prot = Thrift::CompactProtocol.new(trans)
+        prot.write_message_begin('unknown', Thrift::MessageTypes::CALL, 1)
+        trans.write([0x1e, 0].pack('C*'))
+        trans.read(trans.available)
+      end,
+      Thrift::JsonProtocolFactory.new => proc { '[1,"unknown",1,1,{"1":{"wat":0}}]' }
+    }.each do |protocol_factory, malformed_request|
+      it "closes a malformed #{protocol_factory} connection and continues accepting clients" do
+        ready = Queue.new
+        errors = Queue.new
+        server_transport = EphemeralServerSocket.new(ready)
+        handler = StopAfterVoidHandler.new
+        processor = Thrift::Test::Srv::Processor.new(handler)
+        server = Thrift::SimpleServer.new(processor, server_transport, nil, protocol_factory)
+        server_thread = Thread.new do
+          catch(:stop) { server.serve }
+        rescue StandardError, ScriptError => error
+          errors << error
+        end
+        server_thread.report_on_exception = false
+
+        port = Timeout.timeout(2) { ready.pop }
+        malformed_client = TCPSocket.new('127.0.0.1', port)
+        malformed_client.write(malformed_request.call)
+        malformed_client.close_write
+
+        expect(IO.select([malformed_client], nil, nil, 2)).not_to be_nil
+        peer_closed = begin
+          malformed_client.readpartial(1)
+          false
+        rescue EOFError, Errno::ECONNRESET
+          true
+        end
+        expect(peer_closed).to be(true)
+        expect(server_thread).to be_alive
+
+        valid_transport = Thrift::Socket.new('127.0.0.1', port)
+        valid_transport.open
+        valid_protocol = protocol_factory.get_protocol(valid_transport)
+        Thrift::Test::Srv::Client.new(valid_protocol).send_voidMethod
+
+        expect(server_thread.join(2)).to eq(server_thread)
+        expect(handler.calls).to eq(1)
+        expect(errors).to be_empty
+      ensure
+        malformed_client&.close
+        valid_transport&.close
+        server_transport&.close
+        server_thread&.kill
+        server_thread&.join
+      end
+    end
   end
 
   describe Thrift::ThreadedServer do

lib/rb/test/fuzz/fuzz_common.rb

@@ -31,16 +31,11 @@
 
 def ignorable_fuzz_exception?(error)
   return true if error.is_a?(Thrift::ProtocolException) ||
-    error.is_a?(EOFError) ||
-    error.is_a?(Encoding::UndefinedConversionError)
+    error.is_a?(EOFError)
 
   [
-    /don't know what (?:c)?type/,
-    /Too many fields for union/,
     /too big to convert to '(?:int|long)'/,
-    /bignum too big to convert into 'long'/,
-    /negative array size/,
-    /Union fields are not set/
+    /bignum too big to convert into 'long'/
   ].any? { |pattern| error.message =~ pattern }
 end