THRIFT-1182. rb: Native deserializer segfaults on incorrect list element type

This patch causes both the pure ruby and native extension code paths to check if the data in lists, sets, and maps is of the expected type before deserlizing it. When it's not the right type, it now skips the bad data correctly.

Patch: Ilya Maykov

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

diff --git a/lib/rb/ext/struct.c b/lib/rb/ext/struct.c
index db78efa..90c4b05 100644 (file)
--- a/lib/rb/ext/struct.c
+++ b/lib/rb/ext/struct.c
@@ -414,6 +414,8 @@ static VALUE rb_thrift_struct_write(VALUE self, VALUE protocol) {
 
 static VALUE rb_thrift_union_read(VALUE self, VALUE protocol);
 static VALUE rb_thrift_struct_read(VALUE self, VALUE protocol);
+static void skip_map_contents(VALUE protocol, VALUE key_type_value, VALUE value_type_value, int size);
+static void skip_list_or_set_contents(VALUE protocol, VALUE element_type_value, int size);
 
 static void set_field_value(VALUE obj, VALUE field_name, VALUE value) {
   char name_buf[RSTRING_LEN(field_name) + 1];
@@ -424,6 +426,23 @@ static void set_field_value(VALUE obj, VALUE field_name, VALUE value) {
   rb_ivar_set(obj, rb_intern(name_buf), value);
 }
 
+// Helper method to skip the contents of a map (assumes the map header has been read).
+static void skip_map_contents(VALUE protocol, VALUE key_type_value, VALUE value_type_value, int size) {
+  int i;
+  for (i = 0; i < size; i++) {
+    rb_funcall(protocol, skip_method_id, 1, key_type_value);
+    rb_funcall(protocol, skip_method_id, 1, value_type_value);
+  }
+}
+
+// Helper method to skip the contents of a list or set (assumes the list/set header has been read).
+static void skip_list_or_set_contents(VALUE protocol, VALUE element_type_value, int size) {
+  int i;
+  for (i = 0; i < size; i++) {
+    rb_funcall(protocol, skip_method_id, 1, element_type_value);
+  }
+}
+
 static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) {
   VALUE result = Qnil;
 
@@ -458,18 +477,30 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) {
     int value_ttype = FIX2INT(rb_ary_entry(map_header, 1));
     int num_entries = FIX2INT(rb_ary_entry(map_header, 2));
 
+    // Check the declared key and value types against the expected ones and skip the map contents
+    // if the types don't match.
     VALUE key_info = rb_hash_aref(field_info, key_sym);
     VALUE value_info = rb_hash_aref(field_info, value_sym);
 
-    result = rb_hash_new();
+    if (!NIL_P(key_info) && !NIL_P(value_info)) {
+      int specified_key_type = FIX2INT(rb_hash_aref(key_info, type_sym));
+      int specified_value_type = FIX2INT(rb_hash_aref(value_info, type_sym));
+      if (specified_key_type == key_ttype && specified_value_type == value_ttype) {
+        result = rb_hash_new();
 
-    for (i = 0; i < num_entries; ++i) {
-      VALUE key, val;
+        for (i = 0; i < num_entries; ++i) {
+          VALUE key, val;
 
-      key = read_anything(protocol, key_ttype, key_info);
-      val = read_anything(protocol, value_ttype, value_info);
+          key = read_anything(protocol, key_ttype, key_info);
+          val = read_anything(protocol, value_ttype, value_info);
 
-      rb_hash_aset(result, key, val);
+          rb_hash_aset(result, key, val);
+        }
+      } else {
+        skip_map_contents(protocol, INT2FIX(key_ttype), INT2FIX(value_ttype), num_entries);
+      }
+    } else {
+      skip_map_contents(protocol, INT2FIX(key_ttype), INT2FIX(value_ttype), num_entries);
     }
 
     default_read_map_end(protocol);
@@ -479,10 +510,23 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) {
     VALUE list_header = default_read_list_begin(protocol);
     int element_ttype = FIX2INT(rb_ary_entry(list_header, 0));
     int num_elements = FIX2INT(rb_ary_entry(list_header, 1));
-    result = rb_ary_new2(num_elements);
 
-    for (i = 0; i < num_elements; ++i) {
-      rb_ary_push(result, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym)));
+    // Check the declared element type against the expected one and skip the list contents
+    // if the types don't match.
+    VALUE element_info = rb_hash_aref(field_info, element_sym);
+    if (!NIL_P(element_info)) {
+      int specified_element_type = FIX2INT(rb_hash_aref(element_info, type_sym));
+      if (specified_element_type == element_ttype) {
+        result = rb_ary_new2(num_elements);
+
+        for (i = 0; i < num_elements; ++i) {
+          rb_ary_push(result, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym)));
+        }
+      } else {
+        skip_list_or_set_contents(protocol, INT2FIX(element_ttype), num_elements);
+      }
+    } else {
+      skip_list_or_set_contents(protocol, INT2FIX(element_ttype), num_elements);
     }
 
     default_read_list_end(protocol);
@@ -493,15 +537,28 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) {
     VALUE set_header = default_read_set_begin(protocol);
     int element_ttype = FIX2INT(rb_ary_entry(set_header, 0));
     int num_elements = FIX2INT(rb_ary_entry(set_header, 1));
-    items = rb_ary_new2(num_elements);
 
-    for (i = 0; i < num_elements; ++i) {
-      rb_ary_push(items, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym)));
+    // Check the declared element type against the expected one and skip the set contents
+    // if the types don't match.
+    VALUE element_info = rb_hash_aref(field_info, element_sym);
+    if (!NIL_P(element_info)) {
+      int specified_element_type = FIX2INT(rb_hash_aref(element_info, type_sym));
+      if (specified_element_type == element_ttype) {
+        items = rb_ary_new2(num_elements);
+
+        for (i = 0; i < num_elements; ++i) {
+          rb_ary_push(items, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym)));
+        }
+
+        result = rb_class_new_instance(1, &items, rb_cSet);
+      } else {
+        skip_list_or_set_contents(protocol, INT2FIX(element_ttype), num_elements);
+      }
+    } else {
+      skip_list_or_set_contents(protocol, INT2FIX(element_ttype), num_elements);
     }
 
     default_read_set_end(protocol);
-
-    result = rb_class_new_instance(1, &items, rb_cSet);
   } else {
     rb_raise(rb_eNotImpError, "read_anything not implemented for type %d!", ttype);
   }

diff --git a/lib/rb/lib/thrift/struct_union.rb b/lib/rb/lib/thrift/struct_union.rb
index 9bfbed5..849c856 100644 (file)
--- a/lib/rb/lib/thrift/struct_union.rb
+++ b/lib/rb/lib/thrift/struct_union.rb
@@ -46,25 +46,49 @@ module Thrift
         value.read(iprot)
       when Types::MAP
         key_type, val_type, size = iprot.read_map_begin
-        value = {}
-        size.times do
-          k = read_field(iprot, field_info(field[:key]))
-          v = read_field(iprot, field_info(field[:value]))
-          value[k] = v
+        # Skip the map contents if the declared key or value types don't match the expected ones.
+        if (key_type != field[:key][:type] || val_type != field[:value][:type])
+          size.times do
+            iprot.skip(key_type)
+            iprot.skip(val_type)
+          end
+          value = nil
+        else
+          value = {}
+          size.times do
+            k = read_field(iprot, field_info(field[:key]))
+            v = read_field(iprot, field_info(field[:value]))
+            value[k] = v
+          end
         end
         iprot.read_map_end
       when Types::LIST
         e_type, size = iprot.read_list_begin
-        value = Array.new(size) do |n|
-          read_field(iprot, field_info(field[:element]))
+        # Skip the list contents if the declared element type doesn't match the expected one.
+        if (e_type != field[:element][:type])
+          size.times do
+            iprot.skip(e_type)
+          end
+          value = nil
+        else
+          value = Array.new(size) do |n|
+            read_field(iprot, field_info(field[:element]))
+          end
         end
         iprot.read_list_end
       when Types::SET
         e_type, size = iprot.read_set_begin
-        value = Set.new
-        size.times do
-          element = read_field(iprot, field_info(field[:element]))
-          value << element
+        # Skip the set contents if the declared element type doesn't match the expected one.
+        if (e_type != field[:element][:type])
+          size.times do
+            iprot.skip(e_type)
+          end
+        else
+          value = Set.new
+          size.times do
+            element = read_field(iprot, field_info(field[:element]))
+            value << element
+          end
         end
         iprot.read_set_end
       else