read stream to end for negative length in writeBinary(InputStream)

[Sahana2524]

Raw exception from writeBinary when stream length is unknown

writeBinary(Base64Variant, InputStream, int) treats a negative dataLength as a real length, but per the JsonGenerator contract a negative value means "length unknown, read to the end of stream", and the JSON backend already handles it that way. The attribute path reaches new byte[dataLength] and the element path a bounded read(..., dataLength), so the call leaves the generator as a raw NegativeArraySizeException / IndexOutOfBoundsException.

Fix keeps streaming where the code already streams: writeStreamAsBinary(...) now treats len < 0 as "until EOF" and returns the byte count, reading through the recycled base64 buffer (_ioContext.allocBase64Buffer()) instead of 3 bytes per read(). Only the attribute and pretty-printer paths, which need a full buffer for the Stax2 API anyway, read the stream to the end (via ByteArrayBuilder on the buffer recycler). Non-negative lengths keep prior behavior, and writeBinary(..., -1) returns the actual number of bytes written.

Targets 2.x per review, so it can be merged forward.

[github-actions]

🧪 Code Coverage Report

Metric	Coverage	Change
Instructions	74.30%	📈 +0.250%
Branches	69.04%	📈 +0.210%

Coverage data generated from JaCoCo test results

[cowtowncoder]

(Analysis by Claude)

The underlying problem is real and worth fixing. A few things to address before this can go in.

1. Branch target: should this go to 2.x first?

2.x has byte-for-byte the same defect (ToXmlGenerator.writeBinary(Base64Variant, InputStream, int), no negative-length handling before toFullBuffer(...) / the bounded read). Per the usual Jackson flow, a bug like this gets fixed on the oldest maintained branch and merged forward, so this wants to be a 2.x PR rather than 3.x-only.

2. Buffering vs. streaming (the main design question)

For the element path the code already streams via writeStreamAsBinary(...), and the JSON backend streams for unknown length too (WriterBasedJsonGenerator -> _writeBinary(b64variant, data, encodingBuffer) with a recycled buffer). This patch makes unknown length always read the whole stream into a ByteArrayBuilder -- and "length unknown" is exactly the case where the stream may be arbitrarily large. A 1GB stream that works today through the element path would OOM after this change (and toByteArray() briefly doubles the footprint).

Only the attribute and pretty-printer paths genuinely need a full buffer. A narrower fix would keep streaming where it already works -- roughly, teach writeStreamAsBinary(...) to treat len < 0 as "until EOF" and return the byte count:

int chunk = (len < 0) ? (3 - offset) : Math.min(3 - offset, len);

In fairness to the current approach: writeStreamAsBinary(...) reads 3 bytes per read() call, so buffering is actually faster for moderate payloads. That's a reasonable argument, but it also suggests that 3-byte loop deserves its own cleanup rather than being routed around. Either way the memory trade-off should be a deliberate choice.

3. _readAll ignores the buffer recycler

Each call allocates a fresh 4000-byte temp array plus ByteArrayBuilder's own blocks. _ioContext is available in this class:

ByteArrayBuilder bb = new ByteArrayBuilder(_ioContext.bufferRecycler());
byte[] tmp = _ioContext.allocBase64Buffer();   // release in finally

4. Empty stream renders differently from the known-length path

writeBinary(stream, 0) produces <bin/>, while writeBinary(emptyStream, -1) produces <bin></bin> (it inherits the byte[] path's behavior). Semantically equivalent XML, so low severity, but it does contradict the "output identical" claim in the PR description, and it deserves a test pinning whichever behavior we consider correct.

5. Test coverage gaps

Missing cases: empty stream; a payload larger than the 4000-byte temp buffer (the multi-block ByteArrayBuilder path is currently untested); the pretty-printed and unwrapped-element paths; a stream that returns short reads. I verified all of these behave correctly today -- they just aren't locked in by tests.

Nits

• _readAll is inserted between the two toFullBuffer(...) overloads, splitting them up; an overload toFullBuffer(InputStream) placed after them would fit the existing naming.
• The 5-line explanatory comment is longer than house style; the rest of the file uses the dated // DD-Mon-YYYY, tatu: form.
• "hello, binary world".getBytes() uses the platform default charset; XmlTestUtil.utf8Bytes() already exists.
• Test class structure and style otherwise match XmlGeneratorTest conventions nicely.

[github-actions]

🧪 Code Coverage Report

Metric	Coverage	Change
Instructions	74.35%	📈 +0.240%
Branches	68.98%	📈 +0.210%

Coverage data generated from JaCoCo test results

[github-actions]

🧪 Code Coverage Report

Metric	Coverage	Change
Instructions	74.44%	📈 +0.250%
Branches	69.18%	📈 +0.210%

Coverage data generated from JaCoCo test results

[Sahana2524]

Makes sense on all counts. Reworked it and retargeted this PR at 2.x (branch rebuilt on top of 2.x, same fix) so it can be merged forward.

1. Branch: now against 2.x.
2. Buffering vs streaming: agreed, buffering everything was the wrong trade for "length unknown". writeStreamAsBinary(...) now treats len < 0 as "until EOF" and returns the byte count, and only the attribute and pretty-printer paths read to a full buffer. I also took the hint on the 3-byte loop: it reads through the recycled base64 buffer and writes complete triplets per chunk, carrying the remainder. One thing to be aware of: Woodstox starts a fresh encoder per writeBinary() call, so with MIME the line breaks land at chunk-relative positions rather than every 76 chars like the byte[] path (the old 3-byte loop never emitted them at all on the streaming path). Output decodes either way; the large-payload test uses MIME_NO_LINEFEEDS, where stream and byte[] paths come out byte-identical. Happy to go back to the 3-byte loop if you'd rather keep that change separate.
3. Recycler: toFullBuffer(InputStream) uses ByteArrayBuilder(_ioContext.bufferRecycler()) plus allocBase64Buffer(), released in finally.
4. Empty stream: with the streaming path an unknown-length empty stream now renders <bin/>, same as length 0 (and bin="" as attribute); both are pinned in testEmptyStream.
5. Tests: added empty, payload larger than the read buffer (7001 bytes, not a multiple of 3), pretty-printed, unwrapped, and a 1-byte-per-read stream; return values are asserted too.

Nits: toFullBuffer(InputStream) sits after the other two overloads, comment trimmed to the dated form, utf8Bytes() used. Full 2.x suite passes locally (430 tests). I added a 2.23.0 entry to VERSION-2.x/CREDITS-2.x; feel free to drop or reword it.