iemejia commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3568114361
##########
lang/c++/impl/Generic.cc:
##########
@@ -105,11 +267,38 @@ void GenericReader::read(GenericDatum &datum, Decoder &d,
bool isResolving) {
const NodePtr &nn = v.schema()->leafAt(0);
r.resize(0);
size_t start = 0;
+ // Only when not resolving: the datum schema then matches the wire
+ // schema, so minBytesPerElement is a true lower bound. Under
+ // resolution the wire (writer) type may be smaller than the datum
+ // (reader) type, which would over-estimate and reject valid data.
+ int64_t trueMin = minBytesPerElement(nn, 0);
+ // Under resolution the on-wire (writer) element can be zero bytes
+ // even when the reader element is not (e.g. reader-only fields
filled
+ // from defaults), so the bytes check is disabled and we cannot
tell
+ // whether an element is zero-byte on the wire. Apply the tighter
+ // zero-byte cap conservatively in that case, so the up-front
resize
+ // cannot be driven past it.
+ bool zeroByte = isResolving || trueMin == 0;
Review Comment:
This is a deliberate, conservative choice. Under resolution the
bytes-remaining check is disabled (the writer element size may differ from the
reader's), and grow-on-demand alone does not protect the zero-byte case: a
writer array of `null` (0 wire bytes/element) resolving to non-trivial reader
values would let a huge declared count allocate reader elements while consuming
almost no input. Since we can't tell from the reader schema whether the on-wire
element is zero-byte, we apply the tighter item cap to all resolving array
reads. It's not a hard 10,000,000 limit — AVRO_MAX_COLLECTION_ITEMS raises it —
so legitimately large resolving arrays remain readable by configuring the cap.
Tightening this to only the truly-zero-byte-on-wire case would require plumbing
the writer schema into this path; I can file a follow-up if you'd prefer that
refinement.
##########
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:
Left as-is intentionally: an earlier review round had a maintainer remove an
equivalent null-guard here, so re-adding it would conflict with that decision.
In practice remainingBytes() is only meaningful after init()/reset() has
pointed next_/end_ into a real buffer; right after reset() with both null the
value isn't consumed. If the project wants a formal guard I'll defer to a
maintainer call rather than reintroduce the reverted change.
##########
lang/c++/include/avro/Decoder.hh:
##########
@@ -169,6 +169,14 @@ public:
/// by the avro decoder. Similar set of problems occur if the Decoder
/// consumes more than what it should.
virtual void drain() = 0;
+
+ /// Returns the number of bytes that remain to be read from the underlying
+ /// stream, or a negative value when that count is not known (for example a
+ /// streaming source, or a decoder not backed by a byte stream). The
default
+ /// is "unknown"; byte-stream decoders override it so a length prefix or a
+ /// collection block count that exceeds the data actually available can be
+ /// rejected before allocating for it.
+ virtual int64_t bytesRemaining() const { return -1; }
Review Comment:
Valid concern. Adding virtual remainingBytes()/bytesRemaining() to
InputStream/Decoder changes vtable layout and would break ABI for downstream
subclasses across a shared-library boundary. This needs a maintainer decision
on the intended compatibility guarantee: (a) accept it for the next minor/major
(the C++ SDK has historically bumped SONAME on minor releases), or (b) avoid
the vtable change by making these non-virtual with a dynamic_cast to the
concrete decoder/stream where the capability exists. I've left the virtual
approach in place pending that call rather than pick a compatibility policy
unilaterally — happy to switch to the non-virtual + dynamic_cast form if that's
preferred.
--
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]
