Fix incorrect sizing of temp byte arrays in (Unsafe)ByteBufUtil (#8484)

Motivation:

Two similar bugs were introduced by myself in separate recent PRs #8393
and #8464, while optimizing the assignment/handling of temporary arrays
in ByteBufUtil and UnsafeByteBufUtil.

The temp arrays allocated for buffering data written to an OutputStream
are incorrectly sized to the full length of the data to copy rather than
being capped at WRITE_CHUNK_SIZE.

Unfortunately one of these is in the 4.1.31.Final release, I'm really
sorry and will be more careful in future.

This kind of thing is tricky to cover in unit tests.

Modifications:

Revert the temp array allocations back to their original sizes.

Avoid making duplicate calls to ByteBuf.capacity() in a couple of places
in ByteBufUtil (unrelated thing I noticed, can remove it from this PR if
desired!)

Result:

Temporary byte arrays will be reverted to their originally intended
sizes.
This commit is contained in:
Nick Hill 2018-11-09 09:24:38 -08:00 committed by Norman Maurer
parent a140e6dcad
commit 0f8ce1b284
2 changed files with 9 additions and 7 deletions

View File

@ -462,8 +462,9 @@ public final class ByteBufUtil {
}
private static int lastIndexOf(ByteBuf buffer, int fromIndex, int toIndex, byte value) {
fromIndex = Math.min(fromIndex, buffer.capacity());
if (fromIndex < 0 || buffer.capacity() == 0) {
int capacity = buffer.capacity();
fromIndex = Math.min(fromIndex, capacity);
if (fromIndex < 0 || capacity == 0) {
return -1;
}
@ -829,13 +830,14 @@ public final class ByteBufUtil {
* If {@code copy} is false the underlying storage will be shared, if possible.
*/
public static byte[] getBytes(ByteBuf buf, int start, int length, boolean copy) {
if (isOutOfBounds(start, length, buf.capacity())) {
int capacity = buf.capacity();
if (isOutOfBounds(start, length, capacity)) {
throw new IndexOutOfBoundsException("expected: " + "0 <= start(" + start + ") <= start + length(" + length
+ ") <= " + "buf.capacity(" + buf.capacity() + ')');
+ ") <= " + "buf.capacity(" + capacity + ')');
}
if (buf.hasArray()) {
if (copy || start != 0 || length != buf.capacity()) {
if (copy || start != 0 || length != capacity) {
int baseOffset = buf.arrayOffset() + start;
return Arrays.copyOfRange(buf.array(), baseOffset, baseOffset + length);
} else {
@ -1399,7 +1401,7 @@ public final class ByteBufUtil {
buffer.clear().position(position);
if (length <= MAX_TL_ARRAY_LEN || !allocator.isDirectBufferPooled()) {
getBytes(buffer, threadLocalTempArray(length), 0, chunkLen, out, length);
getBytes(buffer, threadLocalTempArray(chunkLen), 0, chunkLen, out, length);
} else {
// if direct buffers are pooled chances are good that heap buffers are pooled as well.
ByteBuf tmpBuf = allocator.heapBuffer(chunkLen);

View File

@ -585,7 +585,7 @@ final class UnsafeByteBufUtil {
if (length != 0) {
int len = Math.min(length, ByteBufUtil.WRITE_CHUNK_SIZE);
if (len <= ByteBufUtil.MAX_TL_ARRAY_LEN || !buf.alloc().isDirectBufferPooled()) {
getBytes(addr, ByteBufUtil.threadLocalTempArray(length), 0, len, out, length);
getBytes(addr, ByteBufUtil.threadLocalTempArray(len), 0, len, out, length);
} else {
// if direct buffers are pooled chances are good that heap buffers are pooled as well.
ByteBuf tmpBuf = buf.alloc().heapBuffer(len);