Copilot commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3567430301
##########
lang/c++/include/avro/Stream.hh:
##########
@@ -345,6 +356,28 @@ struct StreamReader {
return next_ != end_ || fill();
}
+ /**
+ * Returns the number of bytes still available to be read: those already
+ * buffered in this reader plus whatever the underlying stream reports as
+ * remaining. Returns a negative value when the underlying stream cannot
+ * report its remaining size.
+ */
+ int64_t remainingBytes() const {
+ if (in_ == nullptr) {
+ return -1;
+ }
+ int64_t streamRemaining = in_->remainingBytes();
+ if (streamRemaining < 0) {
+ return -1;
+ }
+ // Bytes already buffered in this reader, added to what the underlying
+ // stream still has. When next_ and end_ are both null (right after
+ // init()/reset(), before any data is buffered), the subtraction is
+ // well-defined and yields 0.
+ int64_t buffered = end_ - next_;
Review Comment:
StreamReader::remainingBytes() computes `end_ - next_` even immediately
after reset/init, when both pointers are set to nullptr (see reset() setting
`next_ = end_ = nullptr`). Pointer subtraction is only defined for pointers
into the same array object; subtracting null pointers is undefined behavior and
can make bytesRemaining() unreliable on some platforms/optimizations. Please
guard the subtraction and treat the buffered count as 0 when no buffer has been
filled yet.
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -177,9 +242,27 @@ size_t BinaryDecoder::skipArray() {
for (;;) {
auto r = doDecodeLong();
if (r < 0) {
- auto n = static_cast<size_t>(doDecodeLong());
- in_.skipBytes(n);
+ auto byteSize = doDecodeLong();
+ if (byteSize < 0) {
+ // A negative block byte-size would convert to a huge size_t
and
+ // drive an unbounded skip; reject it.
+ throw Exception("Invalid negative block size: {}", byteSize);
+ }
+ in_.skipBytes(static_cast<size_t>(byteSize));
} else {
+ // Bound the block count: skipping a huge block of zero-byte
elements
+ // would otherwise loop unboundedly (a CPU exhaustion) even though
it
+ // reads/allocates nothing. The decoder has no element schema
here, so
+ // apply the structural cap (AVRO_MAX_COLLECTION_ITEMS, default
+ // Integer.MAX_VALUE - 8). Read the limit each call so a runtime
+ // change to the environment variable is honoured, matching
Generic.cc.
+ const int64_t structural = maxCollectionStructural();
+ if (r > structural) {
+ throw Exception(
+ "Cannot skip a collection of more than {} elements; "
+ "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate",
+ structural);
+ }
return static_cast<size_t>(r);
Review Comment:
skipArray() returns a size_t item count but casts the decoded int64 value
directly (`static_cast<size_t>(r)`). On 32-bit builds (or any platform where
size_t is narrower than int64_t), a large positive block count can truncate and
desynchronize decoding. doDecodeItemCount() already rejects counts that don’t
fit into size_t; skipArray() should apply the same guard before returning.
--
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]
