Copilot commented on code in PR #3594:
URL: https://github.com/apache/thrift/pull/3594#discussion_r3407581461
##########
lib/py/src/ext/protocol.tcc:
##########
@@ -301,9 +301,17 @@ bool ProtocolBase<Impl>::readBytes(char** output, int len)
{
PyErr_Format(PyExc_ValueError, "attempted to read negative length: %d",
len);
return false;
}
- // TODO(dreiss): Don't fear the malloc. Think about taking a copy of
- // the partial read instead of forcing the transport
- // to prepend it to its buffer.
+
+ if (input_.direct_buf) {
+ if (input_.direct_pos + static_cast<size_t>(len) > input_.direct_size) {
+ PyErr_SetString(PyExc_EOFError, "read past end of buffer");
+ return false;
+ }
+
+ *output = const_cast<char*>(input_.direct_buf + input_.direct_pos);
+ input_.direct_pos += static_cast<size_t>(len);
+ return true;
+ }
Review Comment:
The bounds check uses `direct_pos + len > direct_size`, which can overflow
`size_t` on addition and potentially bypass the check in extreme cases. Prefer
an overflow-safe form like `static_cast<size_t>(len) > (input_.direct_size -
input_.direct_pos)` (after ensuring `direct_pos <= direct_size`, which should
hold) to guarantee correct bounds enforcement.
##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -194,8 +195,60 @@ def testMessage(data, strict=True):
return result
+class SimpleStruct(object):
+ thrift_spec = (
+ None,
+ (1, 11, "name", "UTF8", None),
+ (2, 8, "value", None, None),
+ (3, 2, "flag", None, None),
+ )
+
+ def __init__(self, name=None, value=None, flag=None):
+ self.name = name
+ self.value = value
+ self.flag = flag
+
+ def write(self, oprot):
+ if oprot._fast_encode is not None and self.thrift_spec is not None:
+ oprot.trans.write(oprot._fast_encode(self, [self.__class__,
self.thrift_spec]))
+ return
+
+ oprot.writeStructBegin("SimpleStruct")
+ if self.name is not None:
+ oprot.writeFieldBegin("name", 11, 1)
+ oprot.writeString(self.name)
+ oprot.writeFieldEnd()
+ if self.value is not None:
+ oprot.writeFieldBegin("value", 8, 2)
+ oprot.writeI32(self.value)
+ oprot.writeFieldEnd()
+ if self.flag is not None:
+ oprot.writeFieldBegin("flag", 2, 3)
+ oprot.writeBool(self.flag)
+ oprot.writeFieldEnd()
+ oprot.writeFieldStop()
+ oprot.writeStructEnd()
+
+ @classmethod
+ def read(cls, iprot):
+ if (
+ iprot._fast_decode is not None
+ and isinstance(iprot.trans, TTransport.CReadableTransport)
+ and cls.thrift_spec is not None
+ ):
+ return iprot._fast_decode(None, iprot, [cls, cls.thrift_spec])
+ return iprot.readStruct(cls, cls.thrift_spec, False)
Review Comment:
`SimpleStruct.read()` falls back to `iprot.readStruct(...)`, which is not
part of the pure-Python `TBinaryProtocol` API (it’s typically only present on
accelerated protocols). This makes the struct reader unusable outside the
accelerated path and can raise `AttributeError`. Fix by implementing a standard
Thrift struct read loop
(`readStructBegin`/`readFieldBegin`/`skip`/`readStructEnd`) for the fallback
path, or explicitly raise a clear exception when `_fast_decode` isn’t available
and the method is intended to be accelerated-only.
##########
lib/py/src/ext/module.cpp:
##########
@@ -139,11 +139,36 @@ static PyObject* decode_compact(PyObject*, PyObject*
args) {
return decode_impl<CompactProtocol>(args);
}
+static PyObject* decode_binary_from_bytes(PyObject*, PyObject* args) {
+ PyObject* bytes_obj = nullptr;
+ PyObject* typeargs = nullptr;
+ if (!PyArg_ParseTuple(args, "OO", &bytes_obj, &typeargs)) {
+ return nullptr;
+ }
+ if (!PyBytes_Check(bytes_obj)) {
+ PyErr_SetString(PyExc_TypeError, "first argument must be bytes");
+ return nullptr;
+ }
+
+ StructTypeArgs parsedargs;
+ if (!parse_struct_args(&parsedargs, typeargs)) {
+ return nullptr;
+ }
+
+ BinaryProtocol protocol;
+ if (!protocol.prepareDecodeBufferFromBytes(bytes_obj)) {
+ return nullptr;
+ }
+
+ return protocol.readStruct(Py_None, parsedargs.klass, parsedargs.spec);
+}
+
static PyMethodDef ThriftFastBinaryMethods[] = {
{"encode_binary", encode_binary, METH_VARARGS, ""},
{"decode_binary", decode_binary, METH_VARARGS, ""},
{"encode_compact", encode_compact, METH_VARARGS, ""},
{"decode_compact", decode_compact, METH_VARARGS, ""},
+ {"decode_binary_from_bytes", decode_binary_from_bytes, METH_VARARGS, ""},
{nullptr, nullptr, 0, nullptr} /* Sentinel */
};
Review Comment:
The new public API `decode_binary_from_bytes` is exported with an empty
docstring, which makes introspection/help output uninformative. Add a short
docstring describing expected argument types (`bytes` + `[klass, thrift_spec]`)
and the raised exceptions (`TypeError` for non-bytes, `EOFError` for truncated
input).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
