Copilot commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3566909117
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -163,8 +205,14 @@ size_t BinaryDecoder::arrayStart() {
size_t BinaryDecoder::doDecodeItemCount() {
auto result = doDecodeLong();
if (result < 0) {
+ // INT64_MIN cannot be negated in int64_t (it would overflow); reject
it
+ // rather than propagating 2^63 as an item count that inevitably fails
a
+ // huge allocation downstream.
+ if (result == INT64_MIN) {
+ throw Exception("Invalid negative block count: {}", result);
+ }
doDecodeLong();
- return static_cast<size_t>(-(result + 1)) + 1;
+ return static_cast<size_t>(-result);
}
return static_cast<size_t>(result);
Review Comment:
BinaryDecoder::doDecodeItemCount() casts the decoded int64 block count
directly to size_t. On 32-bit builds (or any build where size_t is narrower
than int64_t), large counts can truncate/wrap, which can bypass the downstream
structural caps and lead to incorrect decoding (treating a huge block as a
small one). Please reject counts that do not fit into size_t before returning.
##########
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_;
+ return buffered + streamRemaining;
Review Comment:
StreamReader::remainingBytes() computes `end_ - next_` even when
`next_`/`end_` are null right after init()/reset(). Subtracting null pointers
is undefined behavior, and callers can trigger it via Decoder::bytesRemaining()
immediately after init(). Please guard the pointer subtraction and treat the
buffered count as 0 when no buffer has been filled yet.
--
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]
