diff --git a/lib/rb/README.md b/lib/rb/README.md index f5de53e8602..cfb5b3e2e4f 100644 --- a/lib/rb/README.md +++ b/lib/rb/README.md @@ -169,6 +169,18 @@ certificate must still match the connection hostname. `Thrift::TransportException::ALREADY_OPEN` when the TCP transport is already open. Close the transport before opening it again. +Ruby struct, union, and exception serialization methods now accept an optional +remaining struct depth: `read(protocol, depth = 64)` and +`write(protocol, depth = 64)`. +Unknown and mismatched fields remain independently bounded by +`Thrift::BaseProtocol#skip`. The `write_field` and `write_type` helpers accept +an optional `remaining_depth`: custom writers must use +`write_field(field_info, fid, value, remaining_depth)` or +`write_type(field_info, value, remaining_depth)` for struct values. Custom +protocols and custom struct, union, or exception serialization overrides with +exact argument counts must accept and forward the optional depth argument (or +use `*args`) before upgrading. + ### 0.24.0 Connect timeout handling changed for both `Thrift::Socket` and diff --git a/lib/rb/ext/struct.c b/lib/rb/ext/struct.c index 1db77da3759..1bfc3aedd22 100644 --- a/lib/rb/ext/struct.c +++ b/lib/rb/ext/struct.c @@ -35,6 +35,30 @@ static VALUE default_sym; #define IS_CONTAINER(ttype) ((ttype) == TTYPE_MAP || (ttype) == TTYPE_LIST || (ttype) == TTYPE_SET) #define STRUCT_FIELDS(obj) rb_const_get(CLASS_OF(obj), fields_const_id) +// Default budget when callers do not provide a remaining depth. +static int recursion_limit; + +static void validate_recursion_depth(int remaining_depth) { + if (RB_UNLIKELY(remaining_depth <= 0)) { + rb_exc_raise( + get_protocol_exception( + INT2FIX(PROTOERR_DEPTH_LIMIT), + rb_str_new2("Maximum recursion depth exceeded") + ) + ); + } +} + +static int parse_recursive_args(int argc, const VALUE *argv, VALUE *protocol) { + if (RB_UNLIKELY(argc < 1 || argc > 2)) { + rb_error_arity(argc, 1, 2); + } + + *protocol = argv[0]; + int remaining_depth = argc == 1 ? recursion_limit : NUM2INT(argv[1]); + validate_recursion_depth(remaining_depth); + return remaining_depth; +} static void validate_container_size(int size) { if (RB_UNLIKELY(size < 0)) { @@ -236,9 +260,9 @@ VALUE default_read_struct_end(VALUE protocol) { // end default protocol methods -static VALUE rb_thrift_union_write (VALUE self, VALUE protocol); -static VALUE rb_thrift_struct_write(VALUE self, VALUE protocol); -static void write_anything(int ttype, VALUE value, VALUE protocol, VALUE field_info); +static VALUE rb_thrift_struct_write_recursive(VALUE self, VALUE protocol, int remaining_depth); +static VALUE rb_thrift_union_write_recursive(VALUE self, VALUE protocol, int remaining_depth); +static void write_anything(int ttype, VALUE value, VALUE protocol, VALUE field_info, int remaining_depth); static inline ID field_ivar_id(VALUE field_name) { char name_buf[RSTRING_LEN(field_name) + 2]; @@ -253,7 +277,7 @@ VALUE get_field_value(VALUE obj, VALUE field_name) { return rb_ivar_get(obj, field_ivar_id(field_name)); } -static void write_container(int ttype, VALUE field_info, VALUE value, VALUE protocol) { +static void write_container(int ttype, VALUE field_info, VALUE value, VALUE protocol, int remaining_depth) { long sz, i; if (ttype == TTYPE_MAP) { @@ -280,15 +304,15 @@ static void write_container(int ttype, VALUE field_info, VALUE value, VALUE prot VALUE val = rb_hash_aref(value, key); if (IS_CONTAINER(keytype)) { - write_container(keytype, key_info, key, protocol); + write_container(keytype, key_info, key, protocol, remaining_depth); } else { - write_anything(keytype, key, protocol, key_info); + write_anything(keytype, key, protocol, key_info, remaining_depth); } if (IS_CONTAINER(valuetype)) { - write_container(valuetype, value_info, val, protocol); + write_container(valuetype, value_info, val, protocol, remaining_depth); } else { - write_anything(valuetype, val, protocol, value_info); + write_anything(valuetype, val, protocol, value_info, remaining_depth); } } @@ -306,9 +330,9 @@ static void write_container(int ttype, VALUE field_info, VALUE value, VALUE prot for (i = 0; i < sz; ++i) { VALUE val = rb_ary_entry(value, i); if (IS_CONTAINER(element_type)) { - write_container(element_type, element_type_info, val, protocol); + write_container(element_type, element_type_info, val, protocol, remaining_depth); } else { - write_anything(element_type, val, protocol, element_type_info); + write_anything(element_type, val, protocol, element_type_info, remaining_depth); } } default_write_list_end(protocol); @@ -337,9 +361,9 @@ static void write_container(int ttype, VALUE field_info, VALUE value, VALUE prot for (i = 0; i < sz; i++) { VALUE val = rb_ary_entry(items, i); if (IS_CONTAINER(element_type)) { - write_container(element_type, element_type_info, val, protocol); + write_container(element_type, element_type_info, val, protocol, remaining_depth); } else { - write_anything(element_type, val, protocol, element_type_info); + write_anything(element_type, val, protocol, element_type_info, remaining_depth); } } @@ -349,7 +373,7 @@ static void write_container(int ttype, VALUE field_info, VALUE value, VALUE prot } } -static void write_anything(int ttype, VALUE value, VALUE protocol, VALUE field_info) { +static void write_anything(int ttype, VALUE value, VALUE protocol, VALUE field_info, int remaining_depth) { if (ttype == TTYPE_BOOL) { default_write_bool(protocol, value); } else if (ttype == TTYPE_BYTE) { @@ -372,19 +396,21 @@ static void write_anything(int ttype, VALUE value, VALUE protocol, VALUE field_i } else if (ttype == TTYPE_UUID) { default_write_uuid(protocol, value); } else if (IS_CONTAINER(ttype)) { - write_container(ttype, field_info, value, protocol); + write_container(ttype, field_info, value, protocol, remaining_depth); } else if (ttype == TTYPE_STRUCT) { + remaining_depth--; + validate_recursion_depth(remaining_depth); if (rb_obj_is_kind_of(value, thrift_union_class)) { - rb_thrift_union_write(value, protocol); + rb_thrift_union_write_recursive(value, protocol, remaining_depth); } else { - rb_thrift_struct_write(value, protocol); + rb_thrift_struct_write_recursive(value, protocol, remaining_depth); } } else { rb_raise(rb_eNotImpError, "Unknown type for binary_encoding: %d", ttype); } } -static VALUE rb_thrift_struct_write(VALUE self, VALUE protocol) { +static VALUE rb_thrift_struct_write_recursive(VALUE self, VALUE protocol, int remaining_depth) { // call validate rb_funcall(self, validate_method_id, 0); @@ -410,7 +436,7 @@ static VALUE rb_thrift_struct_write(VALUE self, VALUE protocol) { if (!NIL_P(field_value)) { default_write_field_begin(protocol, field_name, ttype_value, field_id); - write_anything(ttype, field_value, protocol, field_info); + write_anything(ttype, field_value, protocol, field_info, remaining_depth); default_write_field_end(protocol); } @@ -424,12 +450,19 @@ static VALUE rb_thrift_struct_write(VALUE self, VALUE protocol) { return Qnil; } +// cppcheck-suppress constParameterCallback +static VALUE rb_thrift_struct_write(int argc, VALUE *argv, VALUE self) { + VALUE protocol; + int remaining_depth = parse_recursive_args(argc, argv, &protocol); + return rb_thrift_struct_write_recursive(self, protocol, remaining_depth); +} + //------------------------------------------- // Reading section //------------------------------------------- -static VALUE rb_thrift_union_read(VALUE self, VALUE protocol); -static VALUE rb_thrift_struct_read(VALUE self, VALUE protocol); +static VALUE rb_thrift_union_read_recursive(VALUE self, VALUE protocol, int remaining_depth); +static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int remaining_depth); 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); @@ -490,7 +523,7 @@ static void skip_list_or_set_contents(VALUE protocol, VALUE element_type_value, } } -static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) { +static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info, int remaining_depth) { VALUE result = Qnil; if (ttype == TTYPE_BOOL) { @@ -517,11 +550,13 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) { } else if (ttype == TTYPE_STRUCT) { VALUE klass = rb_hash_aref(field_info, class_sym); result = rb_class_new_instance(0, NULL, klass); + remaining_depth--; + validate_recursion_depth(remaining_depth); if (rb_obj_is_kind_of(result, thrift_union_class)) { - rb_thrift_union_read(result, protocol); + rb_thrift_union_read_recursive(result, protocol, remaining_depth); } else { - rb_thrift_struct_read(result, protocol); + rb_thrift_struct_read_recursive(result, protocol, remaining_depth); } } else if (ttype == TTYPE_MAP) { VALUE map_header = default_read_map_begin(protocol); @@ -547,8 +582,8 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) { for (int 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, remaining_depth); + val = read_anything(protocol, value_ttype, value_info, remaining_depth); rb_hash_aset(result, key, val); } @@ -574,7 +609,7 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) { result = new_container_array(num_elements); for (int i = 0; i < num_elements; ++i) { - rb_ary_push(result, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym))); + rb_ary_push(result, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym), remaining_depth)); } } else { validate_container_size(num_elements); @@ -602,7 +637,7 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) { items = new_container_array(num_elements); for (int i = 0; i < num_elements; ++i) { - rb_ary_push(items, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym))); + rb_ary_push(items, read_anything(protocol, element_ttype, rb_hash_aref(field_info, element_sym), remaining_depth)); } result = rb_class_new_instance(1, &items, rb_cSet); @@ -623,7 +658,7 @@ static VALUE read_anything(VALUE protocol, int ttype, VALUE field_info) { return result; } -static VALUE rb_thrift_struct_read(VALUE self, VALUE protocol) { +static VALUE rb_thrift_struct_read_recursive(VALUE self, VALUE protocol, int remaining_depth) { VALUE struct_fields = STRUCT_FIELDS(self); if (RHASH_SIZE(struct_fields) > 0 && rb_ivar_count(self) > 0) { @@ -652,7 +687,7 @@ static VALUE rb_thrift_struct_read(VALUE self, VALUE protocol) { if (field_type == specified_type) { // read the value VALUE name = rb_hash_aref(field_info, name_sym); - set_field_value(self, name, read_anything(protocol, field_type, field_info)); + set_field_value(self, name, read_anything(protocol, field_type, field_info, remaining_depth)); } else { rb_funcall(protocol, skip_method_id, 1, field_type_value); } @@ -673,12 +708,19 @@ static VALUE rb_thrift_struct_read(VALUE self, VALUE protocol) { return Qnil; } +// cppcheck-suppress constParameterCallback +static VALUE rb_thrift_struct_read(int argc, VALUE *argv, VALUE self) { + VALUE protocol; + int remaining_depth = parse_recursive_args(argc, argv, &protocol); + return rb_thrift_struct_read_recursive(self, protocol, remaining_depth); +} + // -------------------------------- // Union section // -------------------------------- -static VALUE rb_thrift_union_read(VALUE self, VALUE protocol) { +static VALUE rb_thrift_union_read_recursive(VALUE self, VALUE protocol, int remaining_depth) { rb_check_frozen(self); if (!NIL_P(rb_ivar_get(self, setfield_id))) { @@ -705,7 +747,7 @@ static VALUE rb_thrift_union_read(VALUE self, VALUE protocol) { if (field_type == specified_type) { // read the value VALUE name = rb_hash_aref(field_info, name_sym); - VALUE value = read_anything(protocol, field_type, field_info); + VALUE value = read_anything(protocol, field_type, field_info, remaining_depth); rb_iv_set(self, "@setfield", rb_str_intern(name)); rb_iv_set(self, "@value", value); } else { @@ -735,7 +777,14 @@ static VALUE rb_thrift_union_read(VALUE self, VALUE protocol) { return Qnil; } -static VALUE rb_thrift_union_write(VALUE self, VALUE protocol) { +// cppcheck-suppress constParameterCallback +static VALUE rb_thrift_union_read(int argc, VALUE *argv, VALUE self) { + VALUE protocol; + int remaining_depth = parse_recursive_args(argc, argv, &protocol); + return rb_thrift_union_read_recursive(self, protocol, remaining_depth); +} + +static VALUE rb_thrift_union_write_recursive(VALUE self, VALUE protocol, int remaining_depth) { // call validate rb_funcall(self, validate_method_id, 0); @@ -759,7 +808,7 @@ static VALUE rb_thrift_union_write(VALUE self, VALUE protocol) { default_write_field_begin(protocol, setfield, ttype_value, field_id); - write_anything(ttype, setvalue, protocol, field_info); + write_anything(ttype, setvalue, protocol, field_info, remaining_depth); default_write_field_end(protocol); @@ -771,17 +820,26 @@ static VALUE rb_thrift_union_write(VALUE self, VALUE protocol) { return Qnil; } +// cppcheck-suppress constParameterCallback +static VALUE rb_thrift_union_write(int argc, VALUE *argv, VALUE self) { + VALUE protocol; + int remaining_depth = parse_recursive_args(argc, argv, &protocol); + return rb_thrift_union_write_recursive(self, protocol, remaining_depth); +} + void Init_struct(void) { VALUE struct_module = rb_const_get(thrift_module, rb_intern("Struct")); - rb_define_method(struct_module, "write", rb_thrift_struct_write, 1); - rb_define_method(struct_module, "read", rb_thrift_struct_read, 1); + recursion_limit = FIX2INT(rb_const_get(thrift_module, rb_intern("DEFAULT_RECURSION_DEPTH"))); + + rb_define_method(struct_module, "write", rb_thrift_struct_write, -1); + rb_define_method(struct_module, "read", rb_thrift_struct_read, -1); thrift_union_class = rb_const_get(thrift_module, rb_intern("Union")); rb_global_variable(&thrift_union_class); - rb_define_method(thrift_union_class, "write", rb_thrift_union_write, 1); - rb_define_method(thrift_union_class, "read", rb_thrift_union_read, 1); + rb_define_method(thrift_union_class, "write", rb_thrift_union_write, -1); + rb_define_method(thrift_union_class, "read", rb_thrift_union_read, -1); setfield_id = rb_intern("@setfield"); rb_global_variable(&setfield_id); diff --git a/lib/rb/lib/thrift/exceptions.rb b/lib/rb/lib/thrift/exceptions.rb index 3f5951b9d07..412dea012a6 100644 --- a/lib/rb/lib/thrift/exceptions.rb +++ b/lib/rb/lib/thrift/exceptions.rb @@ -19,6 +19,8 @@ # module Thrift + DEFAULT_RECURSION_DEPTH = 64 + class Exception < StandardError def initialize(message) super @@ -49,7 +51,8 @@ def initialize(type = UNKNOWN, message = nil) @type = type end - def read(iprot) + def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH) + raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 iprot.read_struct_begin while true fname, ftype, fid = iprot.read_field_begin @@ -68,7 +71,8 @@ def read(iprot) iprot.read_struct_end end - def write(oprot) + def write(oprot, remaining_depth = DEFAULT_RECURSION_DEPTH) + raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 oprot.write_struct_begin('Thrift::ApplicationException') unless @message.nil? oprot.write_field_begin('message', Types::STRING, 1) diff --git a/lib/rb/lib/thrift/protocol/base_protocol.rb b/lib/rb/lib/thrift/protocol/base_protocol.rb index b91defd8efa..a8707aa1fc0 100644 --- a/lib/rb/lib/thrift/protocol/base_protocol.rb +++ b/lib/rb/lib/thrift/protocol/base_protocol.rb @@ -20,6 +20,7 @@ # this require is to make generated struct definitions happy require 'set' +require 'thrift/exceptions' module Thrift class ProtocolException < Exception @@ -236,25 +237,19 @@ def read_uuid # :binary - A Boolean flag that indicates if Thrift::Types::STRING is a binary string (string without encoding). # fid - The ID of the field. # value - The field's value to write; object type varies based on :type. + # remaining_depth - The optional recursion budget of the enclosing struct. # # Returns nothing. - def write_field(*args) - if args.size == 3 - # handles the documented method signature - write_field(field_info, fid, value) - field_info = args[0] - fid = args[1] - value = args[2] - elsif args.size == 4 - # handles the deprecated method signature - write_field(name, type, fid, value) - field_info = {:name => args[0], :type => args[1]} - fid = args[2] - value = args[3] - else - raise ArgumentError, "wrong number of arguments (#{args.size} for 3)" + def write_field(field_info, fid, value, remaining_depth = nil) + unless field_info.is_a?(Hash) + field_info = {:name => field_info, :type => fid} + fid = value + value = remaining_depth + remaining_depth = nil end write_field_begin(field_info[:name], field_info[:type], fid) - write_type(field_info, value) + write_type(field_info, value, remaining_depth) write_field_end end @@ -264,9 +259,10 @@ def write_field(*args) # :type - The Thrift::Types constant that determines how the value is written. # :binary - A Boolean flag that indicates if Thrift::Types::STRING is a binary string (string without encoding). # value - The field's value to write; object type varies based on field_info[:type]. + # remaining_depth - The optional recursion budget of the enclosing struct. # # Returns nothing. - def write_type(field_info, value) + def write_type(field_info, value, remaining_depth = nil) # if field_info is a Integer, assume it is a Thrift::Types constant # convert it into a field_info Hash for backwards compatibility if field_info.is_a? Integer @@ -295,7 +291,11 @@ def write_type(field_info, value) when Types::UUID write_uuid(value) when Types::STRUCT - value.write(self) + if remaining_depth + value.write(self, remaining_depth - 1) + else + value.write(self) + end else raise NotImplementedError end diff --git a/lib/rb/lib/thrift/struct.rb b/lib/rb/lib/thrift/struct.rb index 21c3e26d6aa..ac2d944ea7c 100644 --- a/lib/rb/lib/thrift/struct.rb +++ b/lib/rb/lib/thrift/struct.rb @@ -19,6 +19,7 @@ # require 'set' +require 'thrift/struct_union' module Thrift module Struct @@ -81,7 +82,8 @@ def inspect(skip_optional_nulls = true) "<#{self.class} #{fields.join(", ")}>" end - def read(iprot) + def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH) + raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 unless instance_variables.empty? defaults = fields_with_default_values struct_fields.each_value do |field_info| @@ -96,14 +98,15 @@ def read(iprot) loop do fname, ftype, fid = iprot.read_field_begin break if (ftype == Types::STOP) - handle_message(iprot, fid, ftype) + handle_message(iprot, fid, ftype, remaining_depth) iprot.read_field_end end iprot.read_struct_end validate end - def write(oprot) + def write(oprot, remaining_depth = DEFAULT_RECURSION_DEPTH) + raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 validate oprot.write_struct_begin(self.class.name) each_field do |fid, field_info| @@ -113,8 +116,10 @@ def write(oprot) unless value.nil? if is_container? type oprot.write_field_begin(name, type, fid) - write_container(oprot, value, field_info) + write_container(oprot, value, field_info, remaining_depth) oprot.write_field_end + elsif type == Types::STRUCT + oprot.write_field(field_info, fid, value, remaining_depth) else oprot.write_field(field_info, fid, value) end @@ -235,10 +240,10 @@ def exception_initialize(*args, &block) end end - def handle_message(iprot, fid, ftype) + def handle_message(iprot, fid, ftype, remaining_depth) field = struct_fields[fid] if field and field[:type] == ftype - value = read_field(iprot, field) + value = read_field(iprot, field, remaining_depth) instance_variable_set("@#{field[:name]}", value) else iprot.skip(ftype) diff --git a/lib/rb/lib/thrift/struct_union.rb b/lib/rb/lib/thrift/struct_union.rb index 3cf00d648f9..1f8eaf2f739 100644 --- a/lib/rb/lib/thrift/struct_union.rb +++ b/lib/rb/lib/thrift/struct_union.rb @@ -18,6 +18,8 @@ # under the License. # require 'set' +require 'thrift/exceptions' +require 'thrift/types' module Thrift module Struct_Union @@ -49,11 +51,12 @@ def each_field end end - def read_field(iprot, field = {}) + def read_field(iprot, field = {}, remaining_depth = DEFAULT_RECURSION_DEPTH) case field[:type] when Types::STRUCT + remaining_depth -= 1 value = field[:class].new - value.read(iprot) + value.read(iprot, remaining_depth) when Types::MAP key_type, val_type, size = iprot.read_map_begin raise ProtocolException.new(ProtocolException::NEGATIVE_SIZE, 'Negative size') unless size >= 0 @@ -67,8 +70,8 @@ def read_field(iprot, field = {}) else value = {} size.times do - k = read_field(iprot, field_info(field[:key])) - v = read_field(iprot, field_info(field[:value])) + k = read_field(iprot, field_info(field[:key]), remaining_depth) + v = read_field(iprot, field_info(field[:value]), remaining_depth) value[k] = v end end @@ -85,7 +88,7 @@ def read_field(iprot, field = {}) else value = [] size.times do - value << read_field(iprot, field_info(field[:element])) + value << read_field(iprot, field_info(field[:element]), remaining_depth) end end iprot.read_list_end @@ -100,7 +103,7 @@ def read_field(iprot, field = {}) else value = Set.new size.times do - element = read_field(iprot, field_info(field[:element])) + element = read_field(iprot, field_info(field[:element]), remaining_depth) value << element end end @@ -111,33 +114,36 @@ def read_field(iprot, field = {}) value end - def write_data(oprot, value, field) + def write_data(oprot, value, field, remaining_depth = DEFAULT_RECURSION_DEPTH) if is_container? field[:type] - write_container(oprot, value, field) + write_container(oprot, value, field, remaining_depth) + elsif field[:type] == Types::STRUCT + remaining_depth -= 1 + value.write(oprot, remaining_depth) else oprot.write_type(field, value) end end - def write_container(oprot, value, field = {}) + def write_container(oprot, value, field = {}, remaining_depth = DEFAULT_RECURSION_DEPTH) case field[:type] when Types::MAP oprot.write_map_begin(field[:key][:type], field[:value][:type], value.size) value.each do |k, v| - write_data(oprot, k, field[:key]) - write_data(oprot, v, field[:value]) + write_data(oprot, k, field[:key], remaining_depth) + write_data(oprot, v, field[:value], remaining_depth) end oprot.write_map_end when Types::LIST oprot.write_list_begin(field[:element][:type], value.size) value.each do |elem| - write_data(oprot, elem, field[:element]) + write_data(oprot, elem, field[:element], remaining_depth) end oprot.write_list_end when Types::SET oprot.write_set_begin(field[:element][:type], value.size) value.each do |v,| # the , is to preserve compatibility with the old Hash-style sets - write_data(oprot, v, field[:element]) + write_data(oprot, v, field[:element], remaining_depth) end oprot.write_set_end else @@ -193,5 +199,6 @@ def inspect_collection(collection, field_info) end "[" + buf.join(", ") + "]" end + end end diff --git a/lib/rb/lib/thrift/union.rb b/lib/rb/lib/thrift/union.rb index 9b78bf97af0..6e9adf403ea 100644 --- a/lib/rb/lib/thrift/union.rb +++ b/lib/rb/lib/thrift/union.rb @@ -18,6 +18,8 @@ # under the License. # +require 'thrift/struct_union' + module Thrift class Union def initialize(name = nil, value = nil) @@ -54,13 +56,14 @@ def inspect end end - def read(iprot) + def read(iprot, remaining_depth = DEFAULT_RECURSION_DEPTH) + raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 @setfield = nil @value = nil iprot.read_struct_begin fname, ftype, fid = iprot.read_field_begin - handle_message(iprot, fid, ftype) + handle_message(iprot, fid, ftype, remaining_depth) iprot.read_field_end fname, ftype, fid = iprot.read_field_begin @@ -72,7 +75,8 @@ def read(iprot) validate end - def write(oprot) + def write(oprot, remaining_depth = DEFAULT_RECURSION_DEPTH) + raise ProtocolException.new(ProtocolException::DEPTH_LIMIT, 'Maximum recursion depth exceeded') if remaining_depth <= 0 validate oprot.write_struct_begin(self.class.name) @@ -86,8 +90,10 @@ def write(oprot) type = field_info[:type] if is_container? type oprot.write_field_begin(@setfield, type, fid) - write_container(oprot, @value, field_info) + write_container(oprot, @value, field_info, remaining_depth) oprot.write_field_end + elsif type == Types::STRUCT + oprot.write_field(field_info, fid, @value, remaining_depth) else oprot.write_field(@setfield, type, fid, @value) end @@ -172,10 +178,10 @@ def <=>(other) protected - def handle_message(iprot, fid, ftype) + def handle_message(iprot, fid, ftype, remaining_depth) field = struct_fields[fid] if field and field[:type] == ftype - @value = read_field(iprot, field) + @value = read_field(iprot, field, remaining_depth) name = field[:name].to_sym @setfield = name else diff --git a/lib/rb/spec/ThriftSpec.thrift b/lib/rb/spec/ThriftSpec.thrift index a83014d97cd..4b31824cc32 100644 --- a/lib/rb/spec/ThriftSpec.thrift +++ b/lib/rb/spec/ThriftSpec.thrift @@ -192,6 +192,10 @@ struct RecTree { 2: i16 item } +struct RecStruct { + 1: optional RecStruct child +} + union RecUnion { 1: list children 2: i32 leaf diff --git a/lib/rb/spec/base_protocol_spec.rb b/lib/rb/spec/base_protocol_spec.rb index 29eb4295610..d61ed8bae73 100644 --- a/lib/rb/spec/base_protocol_spec.rb +++ b/lib/rb/spec/base_protocol_spec.rb @@ -52,18 +52,26 @@ it 'should write out a field nicely (deprecated write_field signature)' do expect(@prot).to receive(:write_field_begin).with('field', 'type', 'fid').ordered - expect(@prot).to receive(:write_type).with({:name => 'field', :type => 'type'}, 'value').ordered + expect(@prot).to receive(:write_type).with({:name => 'field', :type => 'type'}, 'value', nil).ordered expect(@prot).to receive(:write_field_end).ordered @prot.write_field('field', 'type', 'fid', 'value') end it 'should write out a field nicely' do expect(@prot).to receive(:write_field_begin).with('field', 'type', 'fid').ordered - expect(@prot).to receive(:write_type).with({:name => 'field', :type => 'type', :binary => false}, 'value').ordered + expect(@prot).to receive(:write_type).with({:name => 'field', :type => 'type', :binary => false}, 'value', nil).ordered expect(@prot).to receive(:write_field_end).ordered @prot.write_field({:name => 'field', :type => 'type', :binary => false}, 'fid', 'value') end + it 'should pass the remaining depth through write_field' do + field = {:name => 'field', :type => 'type'} + expect(@prot).to receive(:write_field_begin).with('field', 'type', 'fid').ordered + expect(@prot).to receive(:write_type).with(field, 'value', 7).ordered + expect(@prot).to receive(:write_field_end).ordered + @prot.write_field(field, 'fid', 'value', 7) + end + it 'should write out the different types (deprecated write_type signature)' do expect(@prot).to receive(:write_bool).with('bool').ordered expect(@prot).to receive(:write_byte).with('byte').ordered @@ -114,6 +122,13 @@ end end + it 'should consume depth when writing a struct type' do + struct = double('Struct') + expect(struct).to receive(:write).with(@prot, 6) + + @prot.write_type({:type => Thrift::Types::STRUCT}, struct, 7) + end + it 'should read the different types (deprecated read_type signature)' do expect(@prot).to receive(:read_bool).ordered expect(@prot).to receive(:read_byte).ordered diff --git a/lib/rb/spec/recursion_depth_spec.rb b/lib/rb/spec/recursion_depth_spec.rb index 5ccb9091b76..d9737aa9f0e 100644 --- a/lib/rb/spec/recursion_depth_spec.rb +++ b/lib/rb/spec/recursion_depth_spec.rb @@ -20,204 +20,348 @@ require 'spec_helper' -# Round-trip test for struct/union/exception recursion depth, driving the -# *generated* read/write path (Thrift::Struct#read/#write, -# Thrift::Union#read/#write) over RecTree / RecUnion / RecError from -# ThriftSpec.thrift; a linear chain is one struct level deeper per node, so a -# chain of N nodes reaches depth N. -# -# NOTE: the Ruby library does not enforce a recursion-depth limit yet -# (THRIFT-6045). The round-trip / within-limit examples are active; the -# limit-enforcement (over-limit) examples are `pending` and will start passing -# once the limit is implemented, at which point RSpec flags them to be enabled. describe 'recursion depth limit' do - # The intended struct/union nesting limit. The Ruby library does not enforce a - # recursion-depth limit yet (THRIFT-6045); the limit-enforcement examples below - # are therefore `pending` until it is implemented. 64 matches the limit other - # Thrift libraries use. - RECURSION_LIMIT = 64 - - # Attached to the pending (over-limit) examples. - PENDING_REASON = 'recursion-depth limit not implemented in the Ruby library yet (THRIFT-6045)' + MAX_DEPTH = Thrift::DEFAULT_RECURSION_DEPTH + OVER_LIMIT = MAX_DEPTH + 1 def binary_protocol Thrift::BinaryProtocol.new(Thrift::MemoryBufferTransport.new) end - # A linearly nested RecTree that is `depth` struct levels deep. - def struct_chain(depth) - node = SpecNamespace::RecTree.new(item: depth, children: []) - node.children = [struct_chain(depth - 1)] if depth > 1 - node - end - - def tree_depth(node) - n = 0 - until node.nil? - n += 1 - break if node.children.nil? || node.children.empty? - node = node.children.first - end - n - end - - # A linearly nested RecUnion that is `depth` levels deep (each union holds the - # next; the innermost holds a scalar leaf). - def union_chain(depth) - if depth > 1 - SpecNamespace::RecUnion.new(children: [union_chain(depth - 1)]) - else - SpecNamespace::RecUnion.new(leaf: 0) - end - end - - # A linearly nested RecError exception that is `depth` struct levels deep. - # Exceptions read/write through the same generated path as structs, so - # tree_depth applies to the decoded chain too. - def error_chain(depth) - node = SpecNamespace::RecError.new(leaf: depth, children: []) - node.children = [error_chain(depth - 1)] if depth > 1 - node - end - - # Emit, via raw protocol calls (which carry no depth guard), the wire image of - # a RecTree chain `depth` levels deep. This lets the read tests feed an - # over-limit payload that the guarded writer would itself refuse to produce. - def write_raw_tree(oprot, depth) - oprot.write_struct_begin('RecTree') - oprot.write_field_begin('children', Thrift::Types::LIST, 1) - oprot.write_list_begin(Thrift::Types::STRUCT, depth > 1 ? 1 : 0) - write_raw_tree(oprot, depth - 1) if depth > 1 - oprot.write_list_end - oprot.write_field_end - oprot.write_field_begin('item', Thrift::Types::I16, 2) - oprot.write_i16(depth) - oprot.write_field_end - oprot.write_field_stop - oprot.write_struct_end - end + it 'accepts an optional recursion depth' do + tree = SpecNamespace::RecTree.new(item: 1, children: []) + protocol = binary_protocol - # Same as write_raw_tree but for the RecError exception (leaf is i32 here). - def write_raw_error(oprot, depth) - oprot.write_struct_begin('RecError') - oprot.write_field_begin('children', Thrift::Types::LIST, 1) - oprot.write_list_begin(Thrift::Types::STRUCT, depth > 1 ? 1 : 0) - write_raw_error(oprot, depth - 1) if depth > 1 - oprot.write_list_end - oprot.write_field_end - oprot.write_field_begin('leaf', Thrift::Types::I32, 2) - oprot.write_i32(depth) - oprot.write_field_end - oprot.write_field_stop - oprot.write_struct_end - end + tree.write(protocol, 1) + expect { SpecNamespace::RecTree.new.read(protocol, 1) }.not_to raise_error - def expect_depth_limit - expect { yield }.to raise_error(Thrift::ProtocolException) { |e| - expect(e.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + expect { tree.read(binary_protocol, 0) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + expect { tree.write(binary_protocol, 0) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) } end - describe 'structs' do + describe 'structs nested in containers' do it 'round-trips a chain at the limit' do - prot = binary_protocol - struct_chain(RECURSION_LIMIT).write(prot) + tree = SpecNamespace::RecTree.new(item: 1, children: []) + 2.upto(MAX_DEPTH) do |depth| + tree = SpecNamespace::RecTree.new(item: depth, children: [tree]) + end + + protocol = binary_protocol + tree.write(protocol) result = SpecNamespace::RecTree.new - result.read(prot) - expect(tree_depth(result)).to eq(RECURSION_LIMIT) + result.read(protocol) + + depth = 0 + until result.nil? + depth += 1 + result = result.children.first + end + expect(depth).to eq(MAX_DEPTH) end it 'rejects writing a chain past the limit' do - pending PENDING_REASON - expect_depth_limit { struct_chain(RECURSION_LIMIT + 1).write(binary_protocol) } + tree = SpecNamespace::RecTree.new(item: 1, children: []) + 2.upto(OVER_LIMIT) do |depth| + tree = SpecNamespace::RecTree.new(item: depth, children: [tree]) + end + + expect { tree.write(binary_protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } end it 'rejects reading a payload past the limit' do - pending PENDING_REASON - prot = binary_protocol - write_raw_tree(prot, RECURSION_LIMIT + 1) - expect_depth_limit { SpecNamespace::RecTree.new.read(prot) } + tree = SpecNamespace::RecTree.new(item: 1, children: []) + 2.upto(OVER_LIMIT) do |depth| + tree = SpecNamespace::RecTree.new(item: depth, children: [tree]) + end + + protocol = binary_protocol + tree.write(protocol, OVER_LIMIT) + + expect { SpecNamespace::RecTree.new.read(protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } end - it 'round-trips a wide shallow tree (counter unwinds per sibling)' do - width = RECURSION_LIMIT * 3 - prot = binary_protocol + it 'round-trips a wide shallow tree' do + width = MAX_DEPTH * 3 root = SpecNamespace::RecTree.new( item: 0, - children: (1..width).map { |i| SpecNamespace::RecTree.new(item: i, children: []) } + children: (1..width).map { |item| SpecNamespace::RecTree.new(item: item, children: []) } ) - root.write(prot) + + protocol = binary_protocol + root.write(protocol) result = SpecNamespace::RecTree.new - result.read(prot) + result.read(protocol) + expect(result.children.size).to eq(width) end + + it 'rejects cyclic values on write' do + root = SpecNamespace::RecTree.new(item: 1, children: []) + root.children = [root] + + expect { root.write(binary_protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + end + end + + describe 'direct struct fields' do + it 'round-trips a chain at the limit' do + tree = SpecNamespace::RecStruct.new + (MAX_DEPTH - 1).times { tree = SpecNamespace::RecStruct.new(child: tree) } + + protocol = binary_protocol + tree.write(protocol) + + expect { SpecNamespace::RecStruct.new.read(protocol) }.not_to raise_error + end + + it 'rejects writing a chain past the limit' do + tree = SpecNamespace::RecStruct.new + (OVER_LIMIT - 1).times { tree = SpecNamespace::RecStruct.new(child: tree) } + + expect { tree.write(binary_protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + end + + it 'preserves the remaining depth through the public field helper' do + parent = SpecNamespace::RecStruct.new(child: SpecNamespace::RecStruct.new) + parent.define_singleton_method(:write) do |oprot, remaining_depth = MAX_DEPTH| + oprot.write_struct_begin(self.class.name) + oprot.write_field(self.class::FIELDS[1], 1, child, remaining_depth) + oprot.write_field_stop + oprot.write_struct_end + end + + expect { parent.write(binary_protocol, 1) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + end + + it 'rejects reading a payload past the limit' do + tree = SpecNamespace::RecStruct.new + (OVER_LIMIT - 1).times { tree = SpecNamespace::RecStruct.new(child: tree) } + + protocol = binary_protocol + tree.write(protocol, OVER_LIMIT) + + expect { SpecNamespace::RecStruct.new.read(protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + end + + it 'uses an independent depth limit for skipped structs' do + write_payload = lambda do |struct_depth| + protocol = binary_protocol + write_struct = lambda do |depth| + protocol.write_struct_begin('UnknownStruct') + if depth > 1 + protocol.write_field_begin('child', Thrift::Types::STRUCT, 1) + write_struct.call(depth - 1) + protocol.write_field_end + end + protocol.write_field_stop + protocol.write_struct_end + end + protocol.write_struct_begin('RecStruct') + protocol.write_field_begin('unknown', Thrift::Types::STRUCT, 2) + write_struct.call(struct_depth) + protocol.write_field_end + protocol.write_field_stop + protocol.write_struct_end + protocol + end + + within_limit = write_payload.call(MAX_DEPTH) + expect { SpecNamespace::RecStruct.new.read(within_limit, 1) }.not_to raise_error + + over_limit = write_payload.call(OVER_LIMIT) + expect { SpecNamespace::RecStruct.new.read(over_limit, 1) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + end + + it 'uses an independent depth limit for skipped containers' do + protocol = binary_protocol + write_list = lambda do |depth| + element_type = depth > 1 ? Thrift::Types::LIST : Thrift::Types::I32 + protocol.write_list_begin(element_type, 1) + if depth > 1 + write_list.call(depth - 1) + else + protocol.write_i32(1) + end + protocol.write_list_end + end + protocol.write_struct_begin('RecStruct') + protocol.write_field_begin('unknown', Thrift::Types::LIST, 2) + write_list.call(3) + protocol.write_field_end + protocol.write_field_stop + protocol.write_struct_end + + expect { SpecNamespace::RecStruct.new.read(protocol, 1) }.not_to raise_error + end + + unless defined? Thrift::BinaryProtocolAccelerated + it 'uses the public field hook for direct structs' do + child = SpecNamespace::RecStruct.new + parent = SpecNamespace::RecStruct.new(child: child) + protocol = binary_protocol + expect(protocol).to receive(:write_field).with(parent.class::FIELDS[1], 1, child, MAX_DEPTH).and_call_original + + parent.write(protocol) + end + + it 'uses the public write hook for nested structs' do + child = SpecNamespace::RecStruct.new + protocol = binary_protocol + expect(child).to receive(:write).with(protocol, MAX_DEPTH - 1).and_call_original + + SpecNamespace::RecStruct.new(child: child).write(protocol) + end + end end describe 'unions' do it 'round-trips a chain at the limit' do - prot = binary_protocol - union_chain(RECURSION_LIMIT).write(prot) - expect { SpecNamespace::RecUnion.new.read(prot) }.not_to raise_error + union = SpecNamespace::RecUnion.new(leaf: 0) + (MAX_DEPTH - 1).times { union = SpecNamespace::RecUnion.new(children: [union]) } + + protocol = binary_protocol + union.write(protocol) + + expect { SpecNamespace::RecUnion.new.read(protocol) }.not_to raise_error end it 'rejects writing a chain past the limit' do - pending PENDING_REASON - expect_depth_limit { union_chain(RECURSION_LIMIT + 1).write(binary_protocol) } + union = SpecNamespace::RecUnion.new(leaf: 0) + (OVER_LIMIT - 1).times { union = SpecNamespace::RecUnion.new(children: [union]) } + + expect { union.write(binary_protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + end + + it 'rejects reading a payload past the limit' do + union = SpecNamespace::RecUnion.new(leaf: 0) + (OVER_LIMIT - 1).times { union = SpecNamespace::RecUnion.new(children: [union]) } + + protocol = binary_protocol + union.write(protocol, OVER_LIMIT) + + expect { SpecNamespace::RecUnion.new.read(protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } end end describe 'exceptions' do it 'round-trips a chain at the limit' do - prot = binary_protocol - error_chain(RECURSION_LIMIT).write(prot) + error = SpecNamespace::RecError.new(leaf: 1, children: []) + 2.upto(MAX_DEPTH) do |depth| + error = SpecNamespace::RecError.new(leaf: depth, children: [error]) + end + + protocol = binary_protocol + error.write(protocol) result = SpecNamespace::RecError.new - result.read(prot) - expect(tree_depth(result)).to eq(RECURSION_LIMIT) + result.read(protocol) + + depth = 0 + until result.nil? + depth += 1 + result = result.children.first + end + expect(depth).to eq(MAX_DEPTH) end it 'rejects writing a chain past the limit' do - pending PENDING_REASON - expect_depth_limit { error_chain(RECURSION_LIMIT + 1).write(binary_protocol) } + error = SpecNamespace::RecError.new(leaf: 1, children: []) + 2.upto(OVER_LIMIT) do |depth| + error = SpecNamespace::RecError.new(leaf: depth, children: [error]) + end + + expect { error.write(binary_protocol) }.to raise_error(Thrift::ProtocolException) { |exception| + expect(exception.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } end it 'rejects reading a payload past the limit' do - pending PENDING_REASON - prot = binary_protocol - write_raw_error(prot, RECURSION_LIMIT + 1) - expect_depth_limit { SpecNamespace::RecError.new.read(prot) } + recursive_error = SpecNamespace::RecError.new(leaf: 1, children: []) + 2.upto(OVER_LIMIT) do |depth| + recursive_error = SpecNamespace::RecError.new(leaf: depth, children: [recursive_error]) + end + + protocol = binary_protocol + recursive_error.write(protocol, OVER_LIMIT) + + expect { SpecNamespace::RecError.new.read(protocol) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } end end - describe 'protocol decorators' do - # A struct written through a decorator must still be bounded and must not - # crash: decorators (MultiplexedProtocol via ProtocolDecorator) do not chain - # BaseProtocol#initialize, so the depth counter starts unset on that object. - it 'round-trips a struct through a MultiplexedProtocol' do - mprot = Thrift::MultiplexedProtocol.new(binary_protocol, 'svc') - struct_chain(3).write(mprot) - result = SpecNamespace::RecTree.new - result.read(mprot) - expect(tree_depth(result)).to eq(3) + describe 'application exceptions' do + it 'accepts and propagates optional recursion depths' do + error = Thrift::ApplicationException.new(Thrift::ApplicationException::UNKNOWN, 'message') + protocol = binary_protocol + + protocol.write_type({:type => Thrift::Types::STRUCT}, error, 2) + expect { + SpecNamespace::RecStruct.new.read_field( + protocol, + {:type => Thrift::Types::STRUCT, :class => Thrift::ApplicationException}, + 2 + ) + }.not_to raise_error + + expect { error.write(binary_protocol, 0) }.to raise_error(Thrift::ProtocolException) { |exception| + expect(exception.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } + expect { error.read(binary_protocol, 0) }.to raise_error(Thrift::ProtocolException) { |exception| + expect(exception.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } end - end - # Only present when the native (thrift_native) extension is loaded, which - # also makes Thrift::Struct#read/#write the native implementations -- the path - # that must enforce the limit in C, not just in pure Ruby. - if defined? Thrift::BinaryProtocolAccelerated - describe 'accelerated binary protocol' do - it 'rejects writing a chain past the limit' do - pending PENDING_REASON - prot = Thrift::BinaryProtocolAccelerated.new(Thrift::MemoryBufferTransport.new) - expect_depth_limit { struct_chain(RECURSION_LIMIT + 1).write(prot) } + it 'uses an independent depth limit for skipped unknown fields' do + write_payload = lambda do |unknown_depth| + protocol = binary_protocol + write_unknown_struct = lambda do |depth| + protocol.write_struct_begin('UnknownStruct') + if depth > 1 + protocol.write_field_begin('child', Thrift::Types::STRUCT, 1) + write_unknown_struct.call(depth - 1) + protocol.write_field_end + end + protocol.write_field_stop + protocol.write_struct_end + end + protocol.write_struct_begin('ApplicationException') + protocol.write_field_begin('unknown', Thrift::Types::STRUCT, 3) + write_unknown_struct.call(unknown_depth) + protocol.write_field_end + protocol.write_field_stop + protocol.write_struct_end + protocol end - it 'rejects reading a payload past the limit' do - pending PENDING_REASON - prot = Thrift::BinaryProtocolAccelerated.new(Thrift::MemoryBufferTransport.new) - write_raw_tree(prot, RECURSION_LIMIT + 1) - expect_depth_limit { SpecNamespace::RecTree.new.read(prot) } - end + within_limit = write_payload.call(MAX_DEPTH) + expect { Thrift::ApplicationException.new.read(within_limit, 1) }.not_to raise_error + + over_limit = write_payload.call(OVER_LIMIT) + expect { Thrift::ApplicationException.new.read(over_limit, 1) }.to raise_error(Thrift::ProtocolException) { |error| + expect(error.type).to eq(Thrift::ProtocolException::DEPTH_LIMIT) + } end end end