THRIFT-547. rb: Thrift deserializer hangs when deserializing empty string

Thrift::MemoryBuffer will now throw an EOFError when it cannot fulfill a request for data.

git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@799696 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/lib/rb/ext/memory_buffer.c b/lib/rb/ext/memory_buffer.c
index 624012d..3a9c101 100644 (file)
--- a/lib/rb/ext/memory_buffer.c
+++ b/lib/rb/ext/memory_buffer.c
@@ -54,6 +54,10 @@ VALUE rb_thrift_memory_buffer_read(VALUE self, VALUE length_value) {
     index = 0;
   }
 
+  if (RSTRING(data)->len < length) {
+    rb_raise(rb_eEOFError, "Not enough bytes remain in memory buffer");
+  }
+
   rb_ivar_set(self, index_ivar_id, INT2FIX(index));
   return data;
 }

diff --git a/lib/rb/lib/thrift/transport/memory_buffer_transport.rb b/lib/rb/lib/thrift/transport/memory_buffer_transport.rb
index 33d732d..b4a80f8 100644 (file)
--- a/lib/rb/lib/thrift/transport/memory_buffer_transport.rb
+++ b/lib/rb/lib/thrift/transport/memory_buffer_transport.rb
@@ -64,6 +64,9 @@ module Thrift
         @buf = @buf.slice(@index..-1)
         @index = 0
       end
+      if data.size < len
+        raise EOFError, "Not enough bytes remain in buffer"
+      end
       data
     end
 

diff --git a/lib/rb/spec/base_transport_spec.rb b/lib/rb/spec/base_transport_spec.rb
index 7189775..a29656f 100644 (file)
--- a/lib/rb/spec/base_transport_spec.rb
+++ b/lib/rb/spec/base_transport_spec.rb
@@ -276,7 +276,7 @@ class ThriftBaseTransportSpec < Spec::ExampleGroup
       @buffer.read(4)
       @buffer.peek.should be_true
       @buffer.available.should == 5
-      @buffer.read(16)
+      @buffer.read(5)
       @buffer.peek.should be_false
       @buffer.available.should == 0
     end
@@ -285,12 +285,12 @@ class ThriftBaseTransportSpec < Spec::ExampleGroup
       @buffer.write "test data"
       @buffer.reset_buffer("foobar")
       @buffer.available.should == 6
-      @buffer.read(10).should == "foobar"
+      @buffer.read(@buffer.available).should == "foobar"
       @buffer.reset_buffer
       @buffer.available.should == 0
     end
 
-    it "should copy the given string whne resetting the buffer" do
+    it "should copy the given string when resetting the buffer" do
       s = "this is a test"
       @buffer.reset_buffer(s)
       @buffer.available.should == 14
@@ -302,11 +302,18 @@ class ThriftBaseTransportSpec < Spec::ExampleGroup
     it "should return from read what was given in write" do
       @buffer.write "test data"
       @buffer.read(4).should == "test"
-      @buffer.read(10).should == " data"
-      @buffer.read(10).should == ""
+      @buffer.read(@buffer.available).should == " data"
       @buffer.write "foo"
       @buffer.write " bar"
-      @buffer.read(10).should == "foo bar"
+      @buffer.read(@buffer.available).should == "foo bar"
+    end
+    
+    it "should throw an EOFError when there isn't enough data in the buffer" do
+      @buffer.reset_buffer("")
+      lambda{@buffer.read(1)}.should raise_error(EOFError)
+
+      @buffer.reset_buffer("1234")
+      lambda{@buffer.read(5)}.should raise_error(EOFError)
     end
   end
 

diff --git a/lib/rb/spec/binary_protocol_spec_shared.rb b/lib/rb/spec/binary_protocol_spec_shared.rb
index c6608e0..84f5920 100644 (file)
--- a/lib/rb/spec/binary_protocol_spec_shared.rb
+++ b/lib/rb/spec/binary_protocol_spec_shared.rb
@@ -41,19 +41,19 @@ shared_examples_for 'a binary protocol' do
 
   it "should write the message header" do
     @prot.write_message_begin('testMessage', Thrift::MessageTypes::CALL, 17)
-    @trans.read(1000).should == [protocol_class.const_get(:VERSION_1) | Thrift::MessageTypes::CALL, "testMessage".size, "testMessage", 17].pack("NNa11N")
+    @trans.read(@trans.available).should == [protocol_class.const_get(:VERSION_1) | Thrift::MessageTypes::CALL, "testMessage".size, "testMessage", 17].pack("NNa11N")
   end
   
   it "should write the message header without version when writes are not strict" do
     @prot = protocol_class.new(@trans, true, false) # no strict write
     @prot.write_message_begin('testMessage', Thrift::MessageTypes::CALL, 17)
-    @trans.read(1000).should == "\000\000\000\vtestMessage\001\000\000\000\021"
+    @trans.read(@trans.available).should == "\000\000\000\vtestMessage\001\000\000\000\021"
   end
   
   it "should write the message header with a version when writes are strict" do
     @prot = protocol_class.new(@trans) # strict write
     @prot.write_message_begin('testMessage', Thrift::MessageTypes::CALL, 17)
-    @trans.read(1000).should == "\200\001\000\001\000\000\000\vtestMessage\000\000\000\021"
+    @trans.read(@trans.available).should == "\200\001\000\001\000\000\000\vtestMessage\000\000\000\021"
   end
   
 
@@ -61,7 +61,7 @@ shared_examples_for 'a binary protocol' do
 
   it "should write the field header" do
     @prot.write_field_begin('foo', Thrift::Types::DOUBLE, 3)
-    @trans.read(1000).should == [Thrift::Types::DOUBLE, 3].pack("cn")
+    @trans.read(@trans.available).should == [Thrift::Types::DOUBLE, 3].pack("cn")
   end
   
   # field footer is a noop
@@ -73,27 +73,27 @@ shared_examples_for 'a binary protocol' do
   
   it "should write the map header" do
     @prot.write_map_begin(Thrift::Types::STRING, Thrift::Types::LIST, 17)
-    @trans.read(1000).should == [Thrift::Types::STRING, Thrift::Types::LIST, 17].pack("ccN");
+    @trans.read(@trans.available).should == [Thrift::Types::STRING, Thrift::Types::LIST, 17].pack("ccN");
   end
    
   # map footer is a noop
   
   it "should write the list header" do
     @prot.write_list_begin(Thrift::Types::I16, 42)
-    @trans.read(1000).should == [Thrift::Types::I16, 42].pack("cN")
+    @trans.read(@trans.available).should == [Thrift::Types::I16, 42].pack("cN")
   end
   
   # list footer is a noop
   
   it "should write the set header" do
     @prot.write_set_begin(Thrift::Types::I16, 42)
-    @trans.read(1000).should == [Thrift::Types::I16, 42].pack("cN")
+    @trans.read(@trans.available).should == [Thrift::Types::I16, 42].pack("cN")
   end
   
   it "should write a bool" do
     @prot.write_bool(true)
     @prot.write_bool(false)
-    @trans.read(1000).should == "\001\000"
+    @trans.read(@trans.available).should == "\001\000"
   end
   
   it "should treat a nil bool as false" do
@@ -130,7 +130,7 @@ shared_examples_for 'a binary protocol' do
     # and try something out of signed range, it should clip
     @prot.write_i16(2**15 + 5)
     
-    @trans.read(1000).should == "\200\000\374\000\000\021\000\000\330\360\006\273\177\377\200\005"
+    @trans.read(@trans.available).should == "\200\000\374\000\000\021\000\000\330\360\006\273\177\377\200\005"
     
     # a Bignum should error
     # lambda { @prot.write_i16(2**65) }.should raise_error(RangeError)
@@ -147,7 +147,7 @@ shared_examples_for 'a binary protocol' do
       @prot.write_i32(i)
     end
     # try something out of signed range, it should clip
-    @trans.read(1000).should == "\200\000\000\000" + "\377\376\037\r" + "\377\377\366\034" + "\377\377\377\375" + "\000\000\000\000" + "\000#\340\203" + "\000\0000+" + "\177\377\377\377"
+    @trans.read(@trans.available).should == "\200\000\000\000" + "\377\376\037\r" + "\377\377\366\034" + "\377\377\377\375" + "\000\000\000\000" + "\000#\340\203" + "\000\0000+" + "\177\377\377\377"
     [2 ** 31 + 5, 2 ** 65 + 5].each do |i|
       lambda { @prot.write_i32(i) }.should raise_error(RangeError)  
     end
@@ -164,7 +164,7 @@ shared_examples_for 'a binary protocol' do
       @prot.write_i64(i)
     end
     # try something out of signed range, it should clip
-    @trans.read(1000).should == ["\200\000\000\000\000\000\000\000",
+    @trans.read(@trans.available).should == ["\200\000\000\000\000\000\000\000",
       "\377\377\364\303\035\244+]",
       "\377\377\377\377\376\231:\341",
       "\377\377\377\377\377\377\377\026",
@@ -185,7 +185,7 @@ shared_examples_for 'a binary protocol' do
     values = [Float::MIN,-1231.15325, -123123.23, -23.23515123, 0, 12351.1325, 523.23, Float::MAX]
     values.each do |f|
       @prot.write_double(f)
-      @trans.read(1000).should == [f].pack("G")
+      @trans.read(@trans.available).should == [f].pack("G")
     end
   end
   
@@ -196,7 +196,7 @@ shared_examples_for 'a binary protocol' do
   it "should write a string" do
     str = "hello world"
     @prot.write_string(str)
-    @trans.read(1000).should == [str.size].pack("N") + str
+    @trans.read(@trans.available).should == [str.size].pack("N") + str
   end
   
   it "should error gracefully when trying to write a nil string" do
@@ -206,13 +206,13 @@ shared_examples_for 'a binary protocol' do
   it "should write the message header without version when writes are not strict" do
     @prot = protocol_class.new(@trans, true, false) # no strict write
     @prot.write_message_begin('testMessage', Thrift::MessageTypes::CALL, 17)
-    @trans.read(1000).should == "\000\000\000\vtestMessage\001\000\000\000\021"
+    @trans.read(@trans.available).should == "\000\000\000\vtestMessage\001\000\000\000\021"
   end
     
   it "should write the message header with a version when writes are strict" do
     @prot = protocol_class.new(@trans) # strict write
     @prot.write_message_begin('testMessage', Thrift::MessageTypes::CALL, 17)
-    @trans.read(1000).should == "\200\001\000\001\000\000\000\vtestMessage\000\000\000\021"
+    @trans.read(@trans.available).should == "\200\001\000\001\000\000\000\vtestMessage\000\000\000\021"
   end
   
   # message footer is a noop