FrankChen021 commented on code in PR #19818:
URL: https://github.com/apache/druid/pull/19818#discussion_r3686782027
##########
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:
Addressed in `ae6d91a223` by validating `numBytes` against both input and
output remaining capacity before either buffer can be mutated. Added focused
regression coverage for negative and oversized lengths.
##########
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:
Addressed in `ae6d91a223` with an explicit nonnegative/remaining-bytes check
before setting the read-only buffer limit. Regression coverage includes
malformed negative and oversized lengths.
##########
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:
Addressed in `ae6d91a223` with an explicit bounds check before setting the
duplicate buffer limit. Regression coverage now exercises negative and
oversized serialized lengths.
##########
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:
Addressed in `ae6d91a223` with an explicit nonnegative/remaining-bytes check
before changing the duplicate buffer limit. The new object-strategy bounds test
covers both invalid cases and verifies the source buffer is unchanged.
##########
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:
No code change is needed here: `Limits.MAX_VERBOSE_WARNINGS` is declared as
a `long`, so Java binary numeric promotion widens both multiplications to
`long` before `Math.toIntExact` is called. The overflow check is therefore
effective. Resolving as not applicable.
##########
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:
Addressed in `ae6d91a223` by validating `numBytes` against
`buffer.remaining()` before setting the read-only buffer limit. Regression
coverage includes both negative and oversized values.
--
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]
