Skip to content

THRIFT-4024, THRIFT-4783: throw when skipping invalid type #1742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeking3 merged 4 commits into from
Feb 14, 2019
Merged

THRIFT-4024, THRIFT-4783: throw when skipping invalid type #1742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37e7bc5
THRIFT-4024: make c_glib throw on unsupported type when skipping
jeking3 Feb 10, 2019
c4df9c2
THRIFT-4024: make cpp throw on unsupported type when skipping
jeking3 Feb 10, 2019
0e903e6
THRIFT-4783: throw on invalid skip (py)
jeking3 Feb 10, 2019
c4f708e
THRIFT-4024: uniform skip behavior on unsupported type
Jens-G Feb 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 47 additions & 25 deletions lib/c_glib/src/thrift/c_glib/protocol/thrift_protocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -469,38 +476,41 @@ 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:
{
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:
Expand All @@ -509,33 +519,45 @@ 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:
{
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 */
Expand Down
12 changes: 4 additions & 8 deletions lib/cpp/src/thrift/protocol/TProtocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions lib/d/src/thrift/protocol/base.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion lib/dart/lib/src/protocol/t_protocol_util.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class TProtocolUtil {
break;

default:
break;
throw new TProtocolError(TProtocolErrorType.INVALID_DATA, "Invalid data");
}
}
}
2 changes: 0 additions & 2 deletions lib/go/thrift/protocol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lib/javame/src/org/apache/thrift/protocol/TProtocolUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ public static void skip(TProtocol prot, byte type, int maxDepth)
break;
}
default:
break;
throw new TProtocolException(TProtocolException.INVALID_DATA,
"Unrecognized type " + type);
}
}
}
3 changes: 0 additions & 3 deletions lib/js/src/thrift.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
8 changes: 5 additions & 3 deletions lib/lua/TProtocol.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -153,6 +151,10 @@ function TProtocolBase:skip(ttype)
self:skip(ettype)
end
self:readListEnd()
else
terror(TProtocolException:new{
message = 'Invalid data'
})
end
end

Expand Down
2 changes: 0 additions & 2 deletions lib/nodejs/lib/thrift/binary_protocol.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions lib/nodejs/lib/thrift/compact_protocol.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions lib/nodejs/lib/thrift/json_protocol.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions lib/ocaml/src/Thrift.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -248,6 +246,7 @@ struct
self#readListEnd)
| T_UTF8 -> ()
| T_UTF16 -> ()
| _ -> raise (Protocol.E (Protocol.INVALID_DATA, "Invalid data"))
end

class virtual factory =
Expand Down
8 changes: 5 additions & 3 deletions lib/py/src/protocol/TProtocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,7 @@ def readUtf8(self):
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()
Expand Down Expand Up @@ -232,6 +230,10 @@ def skip(self, ttype):
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 = (
Expand Down
4 changes: 2 additions & 2 deletions lib/rb/lib/thrift/protocol/base_protocol.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,6 @@ def read_type(field_info)

def skip(type)
case type
when Types::STOP
nil
when Types::BOOL
read_bool
when Types::BYTE
Expand Down Expand Up @@ -367,6 +365,8 @@ def skip(type)
skip(etype)
end
read_list_end
else
raise ProtocolException.new(ProtocolException::INVALID_DATA, 'Invalid data')
end
end

Expand Down
1 change: 0 additions & 1 deletion lib/rb/spec/base_protocol_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@
@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
Expand Down
3 changes: 2 additions & 1 deletion lib/swift/Sources/TProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,9 @@ public extension TProtocol {
try skip(type: elemType)
}
try readListEnd()

default:
return
throw TProtocolError(error: .invalidData, message: "Invalid data")
}
}
}