diff --git a/lib/rb/ext/compact_protocol.c b/lib/rb/ext/compact_protocol.c index 4288e6ebc4..dede3b9c37 100644 --- a/lib/rb/ext/compact_protocol.c +++ b/lib/rb/ext/compact_protocol.c @@ -62,7 +62,42 @@ static int CTYPE_MAP = 0x0B; static int CTYPE_STRUCT = 0x0C; static int CTYPE_UUID = 0x0D; -VALUE rb_thrift_compact_proto_write_i16(VALUE self, VALUE i16); +static uint32_t int_to_zig_zag(int32_t n); +static void write_varint32(VALUE transport, uint32_t n); + +static int64_t checked_integer_range(VALUE value, int64_t min, int64_t max) { + CHECK_NIL(value); + if (RB_UNLIKELY(!RB_INTEGER_TYPE_P(value))) { + rb_raise(rb_eTypeError, "integer argument expected"); + } + + int64_t integer = NUM2LL(value); + if (RB_UNLIKELY(integer < min || integer > max)) { + rb_raise(rb_eRangeError, "integer out of bounds"); + } + + return integer; +} + +static int8_t checked_byte_value(VALUE value) { + return (int8_t)checked_integer_range(value, INT8_MIN, INT8_MAX); +} + +static int16_t checked_i16_value(VALUE value) { + return (int16_t)checked_integer_range(value, INT16_MIN, INT16_MAX); +} + +static int32_t checked_i32_value(VALUE value) { + return (int32_t)checked_integer_range(value, INT32_MIN, INT32_MAX); +} + +static int32_t checked_size_value(VALUE value) { + return (int32_t)checked_integer_range(value, 0, INT32_MAX); +} + +static int64_t checked_i64_value(VALUE value) { + return checked_integer_range(value, INT64_MIN, INT64_MAX); +} // TODO: implement this static int get_compact_type(VALUE type_value) { @@ -104,13 +139,11 @@ static void write_byte_direct(VALUE transport, int8_t b) { WRITE(transport, (char*)&b, 1); } -static void write_field_begin_internal(VALUE self, VALUE type, VALUE id_value, VALUE type_override) { - int id = FIX2INT(id_value); +static void write_field_begin_internal(VALUE self, VALUE type, int16_t id, int8_t type_override) { + int8_t type_to_write = type_override == 0 ? get_compact_type(type) : type_override; int last_id = LAST_ID(self); VALUE transport = GET_TRANSPORT(self); - // if there's a type override, use that. - int8_t type_to_write = RTEST(type_override) ? FIX2INT(type_override) : get_compact_type(type); // check if we can use delta encoding for the field id int diff = id - last_id; if (diff > 0 && diff <= 15) { @@ -119,10 +152,10 @@ static void write_field_begin_internal(VALUE self, VALUE type, VALUE id_value, V } else { // write them separate write_byte_direct(transport, type_to_write & 0x0f); - rb_thrift_compact_proto_write_i16(self, id_value); + write_varint32(transport, int_to_zig_zag(id)); } - SET_LAST_ID(self, id_value); + SET_LAST_ID(self, INT2FIX(id)); } static uint32_t int_to_zig_zag(int32_t n) { @@ -166,11 +199,12 @@ static void write_varint64(VALUE transport, uint64_t n) { } static void write_collection_begin(VALUE transport, VALUE elem_type, VALUE size_value) { - int size = FIX2INT(size_value); + int size = checked_size_value(size_value); + int compact_type = get_compact_type(elem_type); if (size <= 14) { - write_byte_direct(transport, size << 4 | get_compact_type(elem_type)); + write_byte_direct(transport, size << 4 | compact_type); } else { - write_byte_direct(transport, 0xf0 | get_compact_type(elem_type)); + write_byte_direct(transport, 0xf0 | compact_type); write_varint32(transport, size); } } @@ -217,9 +251,10 @@ VALUE rb_thrift_compact_proto_write_set_end(VALUE self) { VALUE rb_thrift_compact_proto_write_message_begin(VALUE self, VALUE name, VALUE type, VALUE seqid) { VALUE transport = GET_TRANSPORT(self); - int32_t seqid_value = FIX2INT(seqid); + uint32_t type_value = (uint32_t)FIX2INT(type); + int32_t seqid_value = checked_i32_value(seqid); write_byte_direct(transport, PROTOCOL_ID); - write_byte_direct(transport, (VERSION & VERSION_MASK) | ((FIX2INT(type) << TYPE_SHIFT_AMOUNT) & TYPE_MASK)); + write_byte_direct(transport, (VERSION & VERSION_MASK) | ((type_value << TYPE_SHIFT_AMOUNT) & TYPE_MASK)); write_varint32(transport, message_seqid_to_varint32(seqid_value)); rb_thrift_compact_proto_write_string(self, name); @@ -227,11 +262,12 @@ VALUE rb_thrift_compact_proto_write_message_begin(VALUE self, VALUE name, VALUE } VALUE rb_thrift_compact_proto_write_field_begin(VALUE self, VALUE name, VALUE type, VALUE id) { + int16_t field_id = checked_i16_value(id); if (FIX2INT(type) == TTYPE_BOOL) { // we want to possibly include the value, so we'll wait. - rb_ivar_set(self, boolean_field_id, rb_ary_new3(2, type, id)); + rb_ivar_set(self, boolean_field_id, rb_ary_new3(2, type, INT2FIX(field_id))); } else { - write_field_begin_internal(self, type, id, Qnil); + write_field_begin_internal(self, type, field_id, 0); } return Qnil; @@ -243,13 +279,15 @@ VALUE rb_thrift_compact_proto_write_field_stop(VALUE self) { } VALUE rb_thrift_compact_proto_write_map_begin(VALUE self, VALUE ktype, VALUE vtype, VALUE size_value) { - int size = FIX2INT(size_value); + int size = checked_size_value(size_value); + int key_type = get_compact_type(ktype); + int value_type = get_compact_type(vtype); VALUE transport = GET_TRANSPORT(self); if (size == 0) { write_byte_direct(transport, 0); } else { write_varint32(transport, size); - write_byte_direct(transport, get_compact_type(ktype) << 4 | get_compact_type(vtype)); + write_byte_direct(transport, key_type << 4 | value_type); } return Qnil; } @@ -272,32 +310,29 @@ VALUE rb_thrift_compact_proto_write_bool(VALUE self, VALUE b) { write_byte_direct(GET_TRANSPORT(self), type); } else { // we haven't written the field header yet - write_field_begin_internal(self, rb_ary_entry(boolean_field, 0), rb_ary_entry(boolean_field, 1), INT2FIX(type)); + write_field_begin_internal(self, rb_ary_entry(boolean_field, 0), FIX2INT(rb_ary_entry(boolean_field, 1)), type); rb_ivar_set(self, boolean_field_id, Qnil); } return Qnil; } VALUE rb_thrift_compact_proto_write_byte(VALUE self, VALUE byte) { - CHECK_NIL(byte); - write_byte_direct(GET_TRANSPORT(self), FIX2INT(byte)); + write_byte_direct(GET_TRANSPORT(self), checked_byte_value(byte)); return Qnil; } VALUE rb_thrift_compact_proto_write_i16(VALUE self, VALUE i16) { - rb_thrift_compact_proto_write_i32(self, i16); + write_varint32(GET_TRANSPORT(self), int_to_zig_zag(checked_i16_value(i16))); return Qnil; } VALUE rb_thrift_compact_proto_write_i32(VALUE self, VALUE i32) { - CHECK_NIL(i32); - write_varint32(GET_TRANSPORT(self), int_to_zig_zag(NUM2INT(i32))); + write_varint32(GET_TRANSPORT(self), int_to_zig_zag(checked_i32_value(i32))); return Qnil; } VALUE rb_thrift_compact_proto_write_i64(VALUE self, VALUE i64) { - CHECK_NIL(i64); - write_varint64(GET_TRANSPORT(self), ll_to_zig_zag(NUM2LL(i64))); + write_varint64(GET_TRANSPORT(self), ll_to_zig_zag(checked_i64_value(i64))); return Qnil; } @@ -331,8 +366,12 @@ VALUE rb_thrift_compact_proto_write_string(VALUE self, VALUE str) { VALUE rb_thrift_compact_proto_write_binary(VALUE self, VALUE buf) { buf = force_binary_encoding(buf); VALUE transport = GET_TRANSPORT(self); - write_varint32(transport, (uint32_t)RSTRING_LEN(buf)); - WRITE(transport, StringValuePtr(buf), RSTRING_LEN(buf)); + long size = RSTRING_LEN(buf); + if (RB_UNLIKELY(size > INT32_MAX)) { + rb_raise(rb_eRangeError, "integer out of bounds"); + } + write_varint32(transport, (uint32_t)size); + WRITE(transport, StringValuePtr(buf), size); return Qnil; } diff --git a/lib/rb/lib/thrift/protocol/compact_protocol.rb b/lib/rb/lib/thrift/protocol/compact_protocol.rb index d25022dd95..755155c38e 100644 --- a/lib/rb/lib/thrift/protocol/compact_protocol.rb +++ b/lib/rb/lib/thrift/protocol/compact_protocol.rb @@ -29,6 +29,14 @@ class CompactProtocol < BaseProtocol TYPE_SHIFT_AMOUNT = 5 MAX_VARINT32_BYTES = 5 # ceil(32/7); matches protobuf wire format MAX_VARINT_BYTES = 10 # ceil(64/7); matches protobuf wire format + BYTE_MIN = -(2**7) + BYTE_MAX = (2**7) - 1 + I16_MIN = -(2**15) + I16_MAX = (2**15) - 1 + I32_MIN = -(2**31) + I32_MAX = (2**31) - 1 + I64_MIN = -(2**63) + I64_MAX = (2**63) - 1 TSTOP = [nil, Types::STOP, 0] @@ -119,8 +127,14 @@ def initialize(transport) end def write_message_begin(name, type, seqid) - write_byte(PROTOCOL_ID) - write_byte((VERSION & VERSION_MASK) | ((type << TYPE_SHIFT_AMOUNT) & TYPE_MASK)) + unless seqid.is_a?(Integer) + raise 'nil argument not allowed!' if seqid.nil? + raise ::TypeError, 'integer argument expected' + end + raise RangeError if seqid < I32_MIN || seqid > I32_MAX + + write_byte_direct(PROTOCOL_ID) + write_byte_direct((VERSION & VERSION_MASK) | ((type << TYPE_SHIFT_AMOUNT) & TYPE_MASK)) write_varint32(message_seqid_to_varint32(seqid)) write_string(name) nil @@ -137,6 +151,12 @@ def write_struct_end end def write_field_begin(name, type, id) + unless id.is_a?(Integer) + raise 'nil argument not allowed!' if id.nil? + raise ::TypeError, 'integer argument expected' + end + raise RangeError if id < I16_MIN || id > I16_MAX + if type == Types::BOOL # we want to possibly include the value, so we'll wait. @boolean_field = [type, id] @@ -155,16 +175,16 @@ def write_field_begin_internal(type, id, type_override = nil) last_id = @last_field.pop # if there's a type override, use that. - typeToWrite = type_override || CompactTypes.get_compact_type(type) + type_to_write = type_override || CompactTypes.get_compact_type(type) # check if we can use delta encoding for the field id if id > last_id && id - last_id <= 15 # write them together - write_byte((id - last_id) << 4 | typeToWrite) + write_byte_direct((id - last_id) << 4 | type_to_write) else # write them separate - write_byte(typeToWrite) - write_i16(id) + write_byte_direct(type_to_write) + write_varint32(int_to_zig_zag(id)) end @last_field.push(id) @@ -172,15 +192,18 @@ def write_field_begin_internal(type, id, type_override = nil) end def write_field_stop - write_byte(Types::STOP) + write_byte_direct(Types::STOP) end def write_map_begin(ktype, vtype, size) + size = validate_size(size) + key_type = CompactTypes.get_compact_type(ktype) + value_type = CompactTypes.get_compact_type(vtype) if (size == 0) - write_byte(0) + write_byte_direct(0) else write_varint32(size) - write_byte(CompactTypes.get_compact_type(ktype) << 4 | CompactTypes.get_compact_type(vtype)) + write_byte_direct(key_type << 4 | value_type) end end @@ -201,23 +224,47 @@ def write_bool(bool) @boolean_field = nil else # we're not part of a field, so just write the value. - write_byte(type) + write_byte_direct(type) end end def write_byte(byte) - @trans.write([byte].pack('c')) + unless byte.is_a?(Integer) + raise 'nil argument not allowed!' if byte.nil? + raise ::TypeError, 'integer argument expected' + end + raise RangeError if byte < BYTE_MIN || byte > BYTE_MAX + + write_byte_direct(byte) end def write_i16(i16) + unless i16.is_a?(Integer) + raise 'nil argument not allowed!' if i16.nil? + raise ::TypeError, 'integer argument expected' + end + raise RangeError if i16 < I16_MIN || i16 > I16_MAX + write_varint32(int_to_zig_zag(i16)) end def write_i32(i32) + unless i32.is_a?(Integer) + raise 'nil argument not allowed!' if i32.nil? + raise ::TypeError, 'integer argument expected' + end + raise RangeError if i32 < I32_MIN || i32 > I32_MAX + write_varint32(int_to_zig_zag(i32)) end def write_i64(i64) + unless i64.is_a?(Integer) + raise 'nil argument not allowed!' if i64.nil? + raise ::TypeError, 'integer argument expected' + end + raise RangeError if i64 < I64_MIN || i64 > I64_MAX + write_varint64(long_to_zig_zag(i64)) end @@ -231,7 +278,8 @@ def write_string(str) end def write_binary(buf) - write_varint32(buf.bytesize) + size = validate_size(buf.bytesize) + write_varint32(size) @trans.write(buf) end @@ -389,44 +437,51 @@ def to_s # the wire differ only by the type indicator. # def write_collection_begin(elem_type, size) + size = validate_size(size) + compact_type = CompactTypes.get_compact_type(elem_type) if size <= 14 - write_byte(size << 4 | CompactTypes.get_compact_type(elem_type)) + write_byte_direct(size << 4 | compact_type) else - write_byte(0xf0 | CompactTypes.get_compact_type(elem_type)) + write_byte_direct(0xf0 | compact_type) write_varint32(size) end end + def write_byte_direct(byte) + @trans.write([byte].pack('C')) + end + def write_varint32(n) - # int idx = 0; - while true - if (n & ~0x7F) == 0 - # i32buf[idx++] = (byte)n; - write_byte(n) - break - # return; - else - # i32buf[idx++] = (byte)((n & 0x7F) | 0x80); - write_byte((n & 0x7F) | 0x80) - n = n >> 7 - end + if (n & ~0x7F) == 0 + write_byte_direct(n) + return end - # trans_.write(i32buf, 0, idx); + + buffer = String.new(capacity: MAX_VARINT32_BYTES, encoding: Encoding::BINARY) + while (n & ~0x7F) != 0 + buffer << ((n & 0x7F) | 0x80) + n >>= 7 + end + buffer << n + @trans.write(buffer) end SEVEN_BIT_MASK = 0x7F EVERYTHING_ELSE_MASK = ~SEVEN_BIT_MASK def write_varint64(n) - while true - if (n & EVERYTHING_ELSE_MASK) == 0 # TODO need to find a way to make this into a long... - write_byte(n) - break - else - write_byte((n & SEVEN_BIT_MASK) | 0x80) - n >>= 7 - end + if (n & EVERYTHING_ELSE_MASK) == 0 + write_byte_direct(n) + return end + + buffer = String.new(capacity: MAX_VARINT_BYTES, encoding: Encoding::BINARY) + while (n & EVERYTHING_ELSE_MASK) != 0 + buffer << ((n & SEVEN_BIT_MASK) | 0x80) + n >>= 7 + end + buffer << n + @trans.write(buffer) end def read_varint32() @@ -471,11 +526,17 @@ def zig_zag_to_long(n) end def message_seqid_to_varint32(seqid) - if seqid < -(2**31) || seqid > (2**31) - 1 - raise RangeError, "seqid must be a signed int32" + seqid < 0 ? seqid + (2**32) : seqid + end + + def validate_size(size) + unless size.is_a?(Integer) + raise 'nil argument not allowed!' if size.nil? + raise ::TypeError, 'integer argument expected' end + raise RangeError if size < 0 || size > I32_MAX - seqid < 0 ? seqid + (2**32) : seqid + size end def message_seqid_from_varint32(seqid) diff --git a/lib/rb/spec/compact_protocol_spec.rb b/lib/rb/spec/compact_protocol_spec.rb index 31d8e432e2..ebdba199f0 100644 --- a/lib/rb/spec/compact_protocol_spec.rb +++ b/lib/rb/spec/compact_protocol_spec.rb @@ -23,6 +23,8 @@ describe Thrift::CompactProtocol do INTEGER_BOUNDARY_TESTS = { + :byte => [-(2**7), (2**7) - 1], + :i16 => [-(2**15), (2**15) - 1], :i32 => [-(2**31), (2**31) - 1], :i64 => [-(2**63), (2**63) - 1] } @@ -100,6 +102,129 @@ end end + it "rejects fixed-width integers outside their declared ranges without writing" do + INTEGER_BOUNDARY_TESTS.each do |primitive_type, (minimum, maximum)| + [minimum - 1, maximum + 1].each do |value| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + + expect { proto.send(writer(primitive_type), value) }.to raise_error(RangeError) + expect(trans.available).to eq(0) + end + end + end + + it "rejects non-integer fixed-width values consistently without writing" do + INTEGER_BOUNDARY_TESTS.each_key do |primitive_type| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + + expect { proto.send(writer(primitive_type), 1.5) }.to raise_error(TypeError, "integer argument expected") + expect(trans.available).to eq(0) + end + end + + it "rejects nil fixed-width values consistently without writing" do + INTEGER_BOUNDARY_TESTS.each_key do |primitive_type| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + + expect { proto.send(writer(primitive_type), nil) }.to raise_error(StandardError, "nil argument not allowed!") + expect(trans.available).to eq(0) + end + end + + it "validates field ids before changing protocol state or wire bytes" do + [-2**15 - 1, 2**15, 1.5, nil].each do |field_id| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + proto.write_struct_begin("Value") + + expected_error = field_id.nil? ? StandardError : field_id.is_a?(Integer) ? RangeError : TypeError + expect { + proto.write_field_begin("value", Thrift::Types::I32, field_id) + }.to raise_error(expected_error) + expect(trans.available).to eq(0) + end + + [-2**15, (2**15) - 1].each do |field_id| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + proto.write_struct_begin("Value") + expect { proto.write_field_begin("value", Thrift::Types::I32, field_id) }.not_to raise_error + end + end + + it "validates message sequence ids before writing the envelope" do + [-2**31 - 1, 2**31, 1.5, nil].each do |sequence_id| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + expected_error = sequence_id.nil? ? StandardError : sequence_id.is_a?(Integer) ? RangeError : TypeError + + expect { + proto.write_message_begin("value", Thrift::MessageTypes::CALL, sequence_id) + }.to raise_error(expected_error) + expect(trans.available).to eq(0) + end + end + + it "writes message types with their exact envelope bytes" do + { + Thrift::MessageTypes::CALL => 0x21, + Thrift::MessageTypes::REPLY => 0x41, + Thrift::MessageTypes::EXCEPTION => 0x61, + Thrift::MessageTypes::ONEWAY => 0x81, + 140 => 0x81 + }.each do |message_type, version_and_type| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + + proto.write_message_begin("", message_type, 0) + + expect(trans.read(trans.available).bytes).to eq([0x82, version_and_type, 0, 0]) + end + end + + it "validates collection sizes before writing their headers" do + writers = [ + [:write_map_begin, [Thrift::Types::STRING, Thrift::Types::I32]], + [:write_list_begin, [Thrift::Types::I16]], + [:write_set_begin, [Thrift::Types::I16]] + ] + + writers.each do |method, arguments| + [-1, 2**31, 1.5, nil].each do |size| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + expected_error = size.nil? ? StandardError : size.is_a?(Integer) ? RangeError : TypeError + + expect { proto.public_send(method, *arguments, size) }.to raise_error(expected_error) + expect(trans.available).to eq(0) + end + + [0, (2**31) - 1].each do |size| + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + expect { proto.public_send(method, *arguments, size) }.not_to raise_error + end + end + end + + it "does not append rejected integers after field and collection headers" do + trans = Thrift::MemoryBufferTransport.new + proto = Thrift::CompactProtocol.new(trans) + proto.write_struct_begin("Value") + proto.write_field_begin("value", Thrift::Types::I16, 1) + field_header_size = trans.available + expect { proto.write_i16(2**15) }.to raise_error(RangeError) + expect(trans.available).to eq(field_header_size) + + proto.write_list_begin(Thrift::Types::I32, 1) + list_header_size = trans.available + expect { proto.write_i32(2**31) }.to raise_error(RangeError) + expect(trans.available).to eq(list_header_size) + end + it "should encode signed integer minima with the canonical zigzag varint bytes" do INTEGER_MINIMUM_ENCODINGS.each_pair do |primitive_type, expected_bytes| trans = Thrift::MemoryBufferTransport.new