Copilot commented on code in PR #19818:
URL: https://github.com/apache/druid/pull/19818#discussion_r3685082111
##########
multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -976,15 +976,15 @@ public void workerWarning(List<MSQErrorReport>
errorReports)
{
// This check safeguards that the controller doesn't run out of memory.
Workers apply their own limiting to
// protect their own memory, and to conserve worker -> controller
bandwidth.
- long numReportsToAddCheck = Math.min(
+ final int numReportsToAddCheck = Math.min(
errorReports.size(),
- Limits.MAX_WORKERS * Limits.MAX_VERBOSE_WARNINGS -
workerWarnings.size()
+ Math.toIntExact(Limits.MAX_WORKERS * Limits.MAX_VERBOSE_WARNINGS -
workerWarnings.size())
);
Review Comment:
`Limits.MAX_WORKERS * Limits.MAX_VERBOSE_WARNINGS` is computed using `int`
arithmetic before being passed to `Math.toIntExact(...)`, so the
multiplication/subtraction can overflow silently and defeat the limit check.
Widen the operands before multiplying so `toIntExact` can actually detect
overflow.
This issue also appears on line 985 of the same file.
##########
processing/src/main/java/org/apache/druid/segment/data/CompressionStrategy.java:
##########
@@ -241,9 +241,10 @@ public static class UncompressedDecompressor implements
Decompressor
public void decompress(ByteBuffer in, int numBytes, ByteBuffer out)
{
final ByteBuffer copyBuffer = in.duplicate();
- copyBuffer.limit(copyBuffer.position() + numBytes);
+ final int newPosition = Math.addExact(copyBuffer.position(), numBytes);
+ copyBuffer.limit(newPosition);
out.put(copyBuffer).flip();
- in.position(in.position() + numBytes);
+ in.position(newPosition);
Review Comment:
`numBytes` is not validated before manipulating limits/positions. If
`numBytes` is negative or exceeds `in.remaining()`/`out.remaining()`, this can
rewind `in`, partially write to `out`, and/or throw after mutating buffers. Add
an explicit bounds check up-front.
##########
processing/src/main/java/org/apache/druid/segment/data/GenericIndexed.java:
##########
@@ -115,7 +115,7 @@ public Class<ByteBuffer> getClazz()
public ByteBuffer fromByteBuffer(final ByteBuffer buffer, final int
numBytes)
{
final ByteBuffer dup = buffer.asReadOnlyBuffer();
- dup.limit(buffer.position() + numBytes);
+ dup.limit(Math.addExact(buffer.position(), numBytes));
return dup;
Review Comment:
`UTF8_STRATEGY.fromByteBuffer` sets the returned buffer limit using
`numBytes` without validating it. If `numBytes` is negative or larger than
`buffer.remaining()`, `dup.limit(...)` can throw or produce a read-only buffer
with an unexpected (possibly rewound) position/limit. Add an explicit range
check for `numBytes` first.
##########
processing/src/main/java/org/apache/druid/query/aggregation/hyperloglog/HyperUniquesSerde.java:
##########
@@ -107,7 +107,7 @@ public HyperLogLogCollector fromByteBuffer(ByteBuffer
buffer, int numBytes)
// make a copy of buffer, because the given buffer is not duplicated
in HyperLogLogCollector.makeCollector() and
// stored in a field.
final ByteBuffer readOnlyBuffer = buffer.asReadOnlyBuffer();
- readOnlyBuffer.limit(readOnlyBuffer.position() + numBytes);
+ readOnlyBuffer.limit(Math.addExact(readOnlyBuffer.position(),
numBytes));
return HyperLogLogCollector.makeCollector(readOnlyBuffer);
Review Comment:
`numBytes` is used to set the read-only buffer limit without validation. If
`numBytes` is negative or larger than the remaining bytes, `limit(...)` can
throw or lead to incorrect slicing. Add a `numBytes >= 0 && numBytes <=
readOnlyBuffer.remaining()` check (similar to `HyperUniquesSerdeForTest`).
##########
processing/src/main/java/org/apache/druid/segment/data/ImmutableRTreeObjectStrategy.java:
##########
@@ -65,7 +65,7 @@ public ImmutableRTree fromByteBuffer(ByteBuffer buffer, int
numBytes)
{
// always create the duplicate buffer for creating the objects as original
buffer may have mutations somewhere else which can corrupt objects
ByteBuffer duplicateBuf = buffer.duplicate();
- duplicateBuf.limit(duplicateBuf.position() + numBytes);
+ duplicateBuf.limit(Math.addExact(duplicateBuf.position(), numBytes));
return new ImmutableRTree(duplicateBuf, bitmapFactory);
Review Comment:
`numBytes` is applied to the duplicated buffer limit without validating that
it is nonnegative and within `buffer.remaining()`. With malformed serialized
data, this can throw or set an invalid/incorrect limit. Add an explicit bounds
check before calling `limit(...)`.
##########
processing/src/main/java/org/apache/druid/query/aggregation/SerializablePairLongStringComplexMetricSerde.java:
##########
@@ -156,7 +156,7 @@ public SerializablePairLongString fromByteBuffer(ByteBuffer
buffer, int numBytes
{
ByteBuffer readOnlyByteBuffer =
buffer.asReadOnlyBuffer().order(buffer.order());
- readOnlyByteBuffer.limit(buffer.position() + numBytes);
+ readOnlyByteBuffer.limit(Math.addExact(buffer.position(), numBytes));
Review Comment:
`fromByteBuffer` sets `readOnlyByteBuffer.limit(buffer.position() +
numBytes)` (now via `addExact`) without validating `numBytes` against the
available bytes. For malformed input, negative/too-large `numBytes` can throw
or slice incorrectly. Add an explicit `numBytes >= 0 && numBytes <=
buffer.remaining()` check before setting the limit.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
