Copilot commented on code in PR #3596:
URL: https://github.com/apache/thrift/pull/3596#discussion_r3407605561
##########
lib/py/src/ext/types.h:
##########
@@ -131,8 +132,48 @@ typedef PyObject EncodeBuffer;
#else
extern const char* refill_signature;
struct EncodeBuffer {
- std::vector<char> buf;
- size_t pos;
+ char* data;
+ size_t size;
+ size_t capacity;
+
+ EncodeBuffer() : data(nullptr), size(0), capacity(0) {}
+
+ ~EncodeBuffer() {
+ if (data) {
+ free(data);
+ }
+ }
Review Comment:
EncodeBuffer owns a raw pointer but does not define/disable copy and move
operations. The compiler-generated copy constructor/assignment will
shallow-copy `data`, which can lead to double-free and memory corruption if an
`EncodeBuffer` is ever copied (even accidentally). Make `EncodeBuffer`
non-copyable (delete copy ctor/assignment) and preferably add a move
ctor/assignment (or store the buffer in `std::unique_ptr<char, Deleter>`), so
ownership is unambiguous.
##########
lib/py/src/ext/types.h:
##########
@@ -131,8 +132,48 @@ typedef PyObject EncodeBuffer;
#else
extern const char* refill_signature;
struct EncodeBuffer {
- std::vector<char> buf;
- size_t pos;
+ char* data;
+ size_t size;
+ size_t capacity;
+
+ EncodeBuffer() : data(nullptr), size(0), capacity(0) {}
+
+ ~EncodeBuffer() {
+ if (data) {
+ free(data);
+ }
+ }
+
+ bool init(size_t initial_capacity) {
+ data = static_cast<char*>(malloc(initial_capacity));
+ if (!data) {
+ return false;
+ }
+ size = 0;
+ capacity = initial_capacity;
+ return true;
+ }
+
+ bool ensure(size_t additional) {
+ size_t needed = size + additional;
+ if (needed <= capacity) {
+ return true;
+ }
+
+ size_t new_capacity = capacity == 0 ? needed : capacity;
+ while (new_capacity < needed) {
+ new_capacity *= 2;
+ }
+
+ char* new_data = static_cast<char*>(realloc(data, new_capacity));
+ if (!new_data) {
+ return false;
+ }
+
+ data = new_data;
+ capacity = new_capacity;
+ return true;
+ }
Review Comment:
The capacity growth logic lacks overflow checks. `size + additional` can
overflow `size_t` and wrap, and `new_capacity *= 2` can overflow as well—either
case can lead to allocating a too-small buffer followed by out-of-bounds writes
in `memcpy`. Add explicit guards (e.g., `if (additional > SIZE_MAX - size)
return false;` and checks before doubling) and fail with a `MemoryError`
upstream when overflow is detected.
##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -167,6 +169,16 @@ def testField(type, data):
protocol.readStructEnd()
+APPLICATION_EXCEPTION_TYPEARGS = [
+ TApplicationException,
+ (
+ None,
+ (1, 11, "message", "UTF8", None),
+ (2, 8, "type", None, None),
+ ),
+]
Review Comment:
`APPLICATION_EXCEPTION_TYPEARGS` is a module-level constant but is defined
as a mutable list. Using an immutable tuple here reduces the risk of accidental
mutation between tests and better matches how Thrift type/spec metadata is
typically represented (tuple-based).
##########
lib/py/src/ext/types.h:
##########
@@ -131,8 +132,48 @@ typedef PyObject EncodeBuffer;
#else
extern const char* refill_signature;
struct EncodeBuffer {
- std::vector<char> buf;
- size_t pos;
+ char* data;
+ size_t size;
+ size_t capacity;
+
+ EncodeBuffer() : data(nullptr), size(0), capacity(0) {}
+
+ ~EncodeBuffer() {
+ if (data) {
+ free(data);
+ }
+ }
+
+ bool init(size_t initial_capacity) {
+ data = static_cast<char*>(malloc(initial_capacity));
+ if (!data) {
+ return false;
+ }
+ size = 0;
+ capacity = initial_capacity;
+ return true;
+ }
Review Comment:
`malloc(0)` is implementation-defined and may return `nullptr` even though
it isn’t an allocation failure. As written, `init(0)` will fail on platforms
where `malloc(0)` returns `nullptr`, potentially causing `new_encode_buffer(0)`
to return `nullptr` unexpectedly. Consider treating an initial capacity of 0 as
a valid empty buffer (e.g., set `data=nullptr`, `capacity=0` and return true)
or normalize `initial_capacity` to at least 1.
--
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]
