Skip to content

[Java] Check var-data string length against the buffer before allocating - #1150

Open
slachiewicz wants to merge 1 commit into
aeron-io:masterfrom
slachiewicz:java-vardata-string-length-check
Open

slachiewicz wants to merge 1 commit into
aeron-io:masterfrom
slachiewicz:java-vardata-string-length-check

Conversation

@slachiewicz

Copy link
Copy Markdown
Contributor

The generated String accessor for var data allocated a byte[] of the wire length and advanced the limit before anything checked that length against the buffer, so a frame whose length prefix is negative or far larger than the message made the decoder allocate up to 2 GB, or fail with OutOfMemoryError, before it read past the buffer. The accessor now rejects such a length with IndexOutOfBoundsException before allocating or moving the limit. The IR codecs are regenerated with the same check.

The other var-data accessors (skip, wrap, Appendable) do not allocate and still rely on the buffer's own bounds checks.

Part of #969.

Verified: VarDataStringLengthTest fails on master (OutOfMemoryError: Requested array size exceeds VM limit) and passes here; sbe-tool checkstyle, tests and verifyJavaIrCodecsInSync pass.

final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
final int headerLength = 2;
final int limit = parentMessage.limit();
final int dataLength = (buffer.getShort(limit, BYTE_ORDER) & 0xFFFF);
if (dataLength < 0 || dataLength > buffer.capacity() - limit - headerLength)
…ing.

The generated String accessor must reject a length that is negative or
runs past the buffer before it allocates the temporary byte[] or moves
the limit; checking after the allocation lets the wire size it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants