Copilot commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3567381352
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -163,8 +206,22 @@ 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;
+ result = -result;
+ }
Review Comment:
BinaryDecoder::doDecodeItemCount() discards the “block byte-size” long for
negative block counts without validating it. A negative block byte-size is
malformed (and already explicitly rejected in skipArray()); rejecting it here
too would keep arrayStart()/mapStart() consistent and fail fast on corrupt
inputs.
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -130,6 +172,7 @@ void BinaryDecoder::skipString() {
void BinaryDecoder::decodeBytes(std::vector<uint8_t> &value) {
size_t len = doDecodeLength();
+ checkAvailableBytes(len);
value.resize(len);
Review Comment:
decodeBytes() validates the declared length via checkAvailableBytes(len),
but skipBytes() still skips without validation. On memory-backed streams,
InputStream::skip clamps at EOF instead of throwing, so skipping can succeed
even when the declared length exceeds available data. Add
checkAvailableBytes(len) to skipBytes() for consistent, fail-fast handling of
malformed/truncated inputs when bytesRemaining() is known.
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -109,12 +138,25 @@ size_t BinaryDecoder::doDecodeLength() {
return len;
}
+void BinaryDecoder::checkAvailableBytes(size_t len) {
+ // Reject a declared length that exceeds the data actually available before
+ // allocating for it, to guard against an out-of-memory attack from a
+ // malicious or truncated input. Only enforced when the stream can report
+ // how many bytes remain.
+ int64_t remaining = in_.remainingBytes();
+ if (remaining >= 0 && static_cast<uint64_t>(len) >
static_cast<uint64_t>(remaining)) {
+ throw Exception(
+ "Length {} exceeds the {} bytes available in the stream", len,
remaining);
+ }
+}
+
void BinaryDecoder::drain() {
in_.drain(false);
}
void BinaryDecoder::decodeString(std::string &value) {
size_t len = doDecodeLength();
+ checkAvailableBytes(len);
value.resize(len);
Review Comment:
BinaryDecoder currently guards decodeString() allocations via
checkAvailableBytes(len), but skipString() still skips the declared length
without validating it against the stream’s known remaining bytes. For
memory-backed streams, InputStream::skip implementations clamp at EOF rather
than throwing, so a malformed/truncated length prefix may be silently accepted
when skipping. Call checkAvailableBytes(len) in skipString() as well so
length-prefixed strings are consistently validated whenever bytesRemaining() is
available.
--
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]
