This is an automated email from the ASF dual-hosted git repository. jking pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/thrift.git
The following commit(s) were added to refs/heads/master by this push: new dbc1f8d THRIFT-4024, THRIFT-4783: throw when skipping invalid type (#1742) dbc1f8d is described below commit dbc1f8def5018ce5d85d38b9875c6c6b6b424478 Author: James E. King III <jk...@apache.org> AuthorDate: Thu Feb 14 16:46:38 2019 -0500 THRIFT-4024, THRIFT-4783: throw when skipping invalid type (#1742) * THRIFT-4024: make c_glib throw on unsupported type when skipping * THRIFT-4783: throw on invalid skip (py) * THRIFT-4024: make cpp throw on unsupported type when skipping * THRIFT-4024: uniform skip behavior on unsupported type --- .../src/thrift/c_glib/protocol/thrift_protocol.c | 72 ++++++++++++++-------- lib/cpp/src/thrift/protocol/TProtocol.h | 12 ++-- lib/d/src/thrift/protocol/base.d | 8 +-- lib/dart/lib/src/protocol/t_protocol_util.dart | 2 +- lib/go/thrift/protocol.go | 2 - .../org/apache/thrift/protocol/TProtocolUtil.java | 3 +- lib/js/src/thrift.js | 3 - lib/lua/TProtocol.lua | 8 ++- lib/nodejs/lib/thrift/binary_protocol.js | 2 - lib/nodejs/lib/thrift/compact_protocol.js | 2 - lib/nodejs/lib/thrift/json_protocol.js | 2 - lib/ocaml/src/Thrift.ml | 3 +- lib/py/src/protocol/TProtocol.py | 8 ++- lib/rb/lib/thrift/protocol/base_protocol.rb | 4 +- lib/rb/spec/base_protocol_spec.rb | 1 - lib/swift/Sources/TProtocol.swift | 3 +- 16 files changed, 73 insertions(+), 62 deletions(-) diff --git a/lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c b/lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c index 8296a8c..6e6ae4d 100644 --- a/lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c +++ b/lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c @@ -419,6 +419,13 @@ thrift_protocol_read_binary (ThriftProtocol *protocol, gpointer *buf, len, error); } +#define THRIFT_SKIP_RESULT_OR_RETURN(_RES, _CALL) \ + { \ + gint32 _x = (_CALL); \ + if (_x < 0) { return _x; } \ + (_RES) += _x; \ + } + gint32 thrift_protocol_skip (ThriftProtocol *protocol, ThriftType type, GError **error) { @@ -469,24 +476,24 @@ thrift_protocol_skip (ThriftProtocol *protocol, ThriftType type, GError **error) gchar *name; gint16 fid; ThriftType ftype; - result += thrift_protocol_read_struct_begin (protocol, &name, error); - + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_struct_begin (protocol, &name, error)) while (1) { - result += thrift_protocol_read_field_begin (protocol, &name, &ftype, - &fid, error); - if (result < 0) - { - return result; - } + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_field_begin (protocol, &name, &ftype, + &fid, error)) if (ftype == T_STOP) { break; } - result += thrift_protocol_skip (protocol, ftype, error); - result += thrift_protocol_read_field_end (protocol, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_skip (protocol, ftype, error)) + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_field_end (protocol, error)) } - result += thrift_protocol_read_struct_end (protocol, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_struct_end (protocol, error)) return result; } case T_SET: @@ -494,13 +501,16 @@ thrift_protocol_skip (ThriftProtocol *protocol, ThriftType type, GError **error) gint32 result = 0; ThriftType elem_type; guint32 i, size; - result += thrift_protocol_read_set_begin (protocol, &elem_type, &size, - error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_set_begin (protocol, &elem_type, &size, + error)) for (i = 0; i < size; i++) { - result += thrift_protocol_skip (protocol, elem_type, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_skip (protocol, elem_type, error)) } - result += thrift_protocol_read_set_end (protocol, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_set_end (protocol, error)) return result; } case T_MAP: @@ -509,14 +519,18 @@ thrift_protocol_skip (ThriftProtocol *protocol, ThriftType type, GError **error) ThriftType elem_type; ThriftType key_type; guint32 i, size; - result += thrift_protocol_read_map_begin (protocol, &key_type, &elem_type, &size, - error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_map_begin (protocol, &key_type, &elem_type, &size, + error)) for (i = 0; i < size; i++) { - result += thrift_protocol_skip (protocol, key_type, error); - result += thrift_protocol_skip (protocol, elem_type, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_skip (protocol, key_type, error)) + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_skip (protocol, elem_type, error)) } - result += thrift_protocol_read_map_end (protocol, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_map_end (protocol, error)) return result; } case T_LIST: @@ -524,18 +538,26 @@ thrift_protocol_skip (ThriftProtocol *protocol, ThriftType type, GError **error) gint32 result = 0; ThriftType elem_type; guint32 i, size; - result += thrift_protocol_read_list_begin (protocol, &elem_type, &size, - error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_list_begin (protocol, &elem_type, &size, + error)) for (i = 0; i < size; i++) { - result += thrift_protocol_skip (protocol, elem_type, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_skip (protocol, elem_type, error)) } - result += thrift_protocol_read_list_end (protocol, error); + THRIFT_SKIP_RESULT_OR_RETURN(result, + thrift_protocol_read_list_end (protocol, error)) return result; } default: - return 0; + break; } + + g_set_error (error, THRIFT_PROTOCOL_ERROR, + THRIFT_PROTOCOL_ERROR_INVALID_DATA, + "unrecognized type"); + return -1; } /* define the GError domain for Thrift protocols */ diff --git a/lib/cpp/src/thrift/protocol/TProtocol.h b/lib/cpp/src/thrift/protocol/TProtocol.h index a38660f..7566a25 100644 --- a/lib/cpp/src/thrift/protocol/TProtocol.h +++ b/lib/cpp/src/thrift/protocol/TProtocol.h @@ -746,16 +746,12 @@ uint32_t skip(Protocol_& prot, TType type) { result += prot.readListEnd(); return result; } - case T_STOP: - case T_VOID: - case T_U64: - case T_UTF8: - case T_UTF16: - break; default: - throw TProtocolException(TProtocolException::INVALID_DATA); + break; } - return 0; + + throw TProtocolException(TProtocolException::INVALID_DATA, + "invalid TType"); } }}} // apache::thrift::protocol diff --git a/lib/d/src/thrift/protocol/base.d b/lib/d/src/thrift/protocol/base.d index 70648b3..5b6d845 100644 --- a/lib/d/src/thrift/protocol/base.d +++ b/lib/d/src/thrift/protocol/base.d @@ -260,7 +260,7 @@ protected: * in generated code. */ void skip(Protocol)(Protocol prot, TType type) if (is(Protocol : TProtocol)) { - final switch (type) { + switch (type) { case TType.BOOL: prot.readBool(); break; @@ -324,9 +324,9 @@ void skip(Protocol)(Protocol prot, TType type) if (is(Protocol : TProtocol)) { } prot.readSetEnd(); break; - case TType.STOP: goto case; - case TType.VOID: - assert(false, "Invalid field type passed."); + + default: + throw new TProtocolException(TProtocolException.Type.INVALID_DATA); } } diff --git a/lib/dart/lib/src/protocol/t_protocol_util.dart b/lib/dart/lib/src/protocol/t_protocol_util.dart index ad20068..841ea82 100644 --- a/lib/dart/lib/src/protocol/t_protocol_util.dart +++ b/lib/dart/lib/src/protocol/t_protocol_util.dart @@ -101,7 +101,7 @@ class TProtocolUtil { break; default: - break; + throw new TProtocolError(TProtocolErrorType.INVALID_DATA, "Invalid data"); } } } diff --git a/lib/go/thrift/protocol.go b/lib/go/thrift/protocol.go index 615b7a4..2e6bc4b 100644 --- a/lib/go/thrift/protocol.go +++ b/lib/go/thrift/protocol.go @@ -96,8 +96,6 @@ func Skip(self TProtocol, fieldType TType, maxDepth int) (err error) { } switch fieldType { - case STOP: - return case BOOL: _, err = self.ReadBool() return diff --git a/lib/javame/src/org/apache/thrift/protocol/TProtocolUtil.java b/lib/javame/src/org/apache/thrift/protocol/TProtocolUtil.java index 9bf10f6..c327448 100644 --- a/lib/javame/src/org/apache/thrift/protocol/TProtocolUtil.java +++ b/lib/javame/src/org/apache/thrift/protocol/TProtocolUtil.java @@ -152,7 +152,8 @@ public class TProtocolUtil { break; } default: - break; + throw new TProtocolException(TProtocolException.INVALID_DATA, + "Unrecognized type " + type); } } } diff --git a/lib/js/src/thrift.js b/lib/js/src/thrift.js index 0b4a292..21a3d65 100644 --- a/lib/js/src/thrift.js +++ b/lib/js/src/thrift.js @@ -1434,9 +1434,6 @@ Thrift.Protocol.prototype = { skip: function(type) { var ret, i; switch (type) { - case Thrift.Type.STOP: - return null; - case Thrift.Type.BOOL: return this.readBool(); diff --git a/lib/lua/TProtocol.lua b/lib/lua/TProtocol.lua index 616e167..1306fb3 100644 --- a/lib/lua/TProtocol.lua +++ b/lib/lua/TProtocol.lua @@ -107,9 +107,7 @@ function TProtocolBase:readDouble() end function TProtocolBase:readString() end function TProtocolBase:skip(ttype) - if type == TType.STOP then - return - elseif ttype == TType.BOOL then + if ttype == TType.BOOL then self:readBool() elseif ttype == TType.BYTE then self:readByte() @@ -153,6 +151,10 @@ function TProtocolBase:skip(ttype) self:skip(ettype) end self:readListEnd() + else + terror(TProtocolException:new{ + message = 'Invalid data' + }) end end diff --git a/lib/nodejs/lib/thrift/binary_protocol.js b/lib/nodejs/lib/thrift/binary_protocol.js index b57c8c5..6ab9c05 100644 --- a/lib/nodejs/lib/thrift/binary_protocol.js +++ b/lib/nodejs/lib/thrift/binary_protocol.js @@ -302,8 +302,6 @@ TBinaryProtocol.prototype.getTransport = function() { TBinaryProtocol.prototype.skip = function(type) { switch (type) { - case Type.STOP: - return; case Type.BOOL: this.readBool(); break; diff --git a/lib/nodejs/lib/thrift/compact_protocol.js b/lib/nodejs/lib/thrift/compact_protocol.js index 5c531e5..302a88d 100644 --- a/lib/nodejs/lib/thrift/compact_protocol.js +++ b/lib/nodejs/lib/thrift/compact_protocol.js @@ -854,8 +854,6 @@ TCompactProtocol.prototype.zigzagToI64 = function(n) { TCompactProtocol.prototype.skip = function(type) { switch (type) { - case Type.STOP: - return; case Type.BOOL: this.readBool(); break; diff --git a/lib/nodejs/lib/thrift/json_protocol.js b/lib/nodejs/lib/thrift/json_protocol.js index 727a3b2..7e2b7c9 100644 --- a/lib/nodejs/lib/thrift/json_protocol.js +++ b/lib/nodejs/lib/thrift/json_protocol.js @@ -738,8 +738,6 @@ TJSONProtocol.prototype.getTransport = function() { */ TJSONProtocol.prototype.skip = function(type) { switch (type) { - case Type.STOP: - return; case Type.BOOL: this.readBool(); break; diff --git a/lib/ocaml/src/Thrift.ml b/lib/ocaml/src/Thrift.ml index f0d7a42..063459b 100644 --- a/lib/ocaml/src/Thrift.ml +++ b/lib/ocaml/src/Thrift.ml @@ -206,8 +206,6 @@ struct (* skippage *) method skip typ = match typ with - | T_STOP -> () - | T_VOID -> () | T_BOOL -> ignore self#readBool | T_BYTE | T_I08 -> ignore self#readByte @@ -248,6 +246,7 @@ struct self#readListEnd) | T_UTF8 -> () | T_UTF16 -> () + | _ -> raise (Protocol.E (Protocol.INVALID_DATA, "Invalid data")) end class virtual factory = diff --git a/lib/py/src/protocol/TProtocol.py b/lib/py/src/protocol/TProtocol.py index 8314cf6..3456e8f 100644 --- a/lib/py/src/protocol/TProtocol.py +++ b/lib/py/src/protocol/TProtocol.py @@ -191,9 +191,7 @@ class TProtocolBase(object): return self.readString().decode('utf8') def skip(self, ttype): - if ttype == TType.STOP: - return - elif ttype == TType.BOOL: + if ttype == TType.BOOL: self.readBool() elif ttype == TType.BYTE: self.readByte() @@ -232,6 +230,10 @@ class TProtocolBase(object): for i in range(size): self.skip(etype) self.readListEnd() + else: + raise TProtocolException( + TProtocolException.INVALID_DATA, + "invalid TType") # tuple of: ( 'reader method' name, is_container bool, 'writer_method' name ) _TTYPE_HANDLERS = ( diff --git a/lib/rb/lib/thrift/protocol/base_protocol.rb b/lib/rb/lib/thrift/protocol/base_protocol.rb index 5c693e9..4d83a21 100644 --- a/lib/rb/lib/thrift/protocol/base_protocol.rb +++ b/lib/rb/lib/thrift/protocol/base_protocol.rb @@ -323,8 +323,6 @@ module Thrift def skip(type) case type - when Types::STOP - nil when Types::BOOL read_bool when Types::BYTE @@ -367,6 +365,8 @@ module Thrift skip(etype) end read_list_end + else + raise ProtocolException.new(ProtocolException::INVALID_DATA, 'Invalid data') end end diff --git a/lib/rb/spec/base_protocol_spec.rb b/lib/rb/spec/base_protocol_spec.rb index eca936b..cfa7573 100644 --- a/lib/rb/spec/base_protocol_spec.rb +++ b/lib/rb/spec/base_protocol_spec.rb @@ -163,7 +163,6 @@ describe 'BaseProtocol' do @prot.skip(Thrift::Types::I64) @prot.skip(Thrift::Types::DOUBLE) @prot.skip(Thrift::Types::STRING) - @prot.skip(Thrift::Types::STOP) # should do absolutely nothing end it "should skip structs" do diff --git a/lib/swift/Sources/TProtocol.swift b/lib/swift/Sources/TProtocol.swift index a4e4a20..b111e71 100644 --- a/lib/swift/Sources/TProtocol.swift +++ b/lib/swift/Sources/TProtocol.swift @@ -175,8 +175,9 @@ public extension TProtocol { try skip(type: elemType) } try readListEnd() + default: - return + throw TProtocolError(error: .invalidData, message: "Invalid data") } } }