Fix buffer capacity race condition in spatial#16931
Fix buffer capacity race condition in spatial#16931clintropolis merged 4 commits intoapache:masterfrom
Conversation
| initialOffset, | ||
| data.getInt(childrenOffset + (count++) * Integer.BYTES), | ||
| data, | ||
| data.duplicate(), |
There was a problem hiding this comment.
it looks like most of the usages of ByteBuffer by ImmutableFloatNode use calls that specify the offset in the buffer except for getImmutableBitmap(), which sets the position and slices it (for some reason, it seems like this method in general could be cleaned up). I wonder if we just change that instead of calling duplicate inside the iterator, so that way it is a bit lazier?
@Override
public ImmutableBitmap getImmutableBitmap()
{
final int sizePosition = initialOffset + offsetFromInitial + HEADER_NUM_BYTES + 2 * numDims * Float.BYTES;
int numBytes = data.getInt(sizePosition);
final ByteBuffer readOnlyBuffer = data.asReadOnlyBuffer();
readOnlyBuffer.position(sizePosition + Integer.BYTES);
readOnlyBuffer.limit(readOnlyBuffer.position() + numBytes);
return bitmapFactory.mapImmutableBitmap(readOnlyBuffer);
}
This is consistent with how getCoords works, which also does something similar of making a readonly copy.
There was a problem hiding this comment.
Though the current way, also avoids any potential misuse of the buffer in this class.
There was a problem hiding this comment.
yea, but it would call duplicate for every entry instead of just the ones that satisfy the bounds check that we decided to use the bitmap
There was a problem hiding this comment.
@clintropolis Thank you Clint, I liked the suggestion as well as it can avoid creating duplicate references on each tree node and sometimes tree can be really big.
|
@clintropolis @abhishekagarwal87 Can you please review the additional instrumentations? |
| log.error(e, "Failed to read bitmap from buffer '%s'," | ||
| + "current pos: %d, set pos: %d, set limit: %d", | ||
| Base64.getEncoder().encodeToString(readOnlyBuffer.array()), | ||
| readOnlyBuffer.position(), | ||
| newPosition, | ||
| newLimit | ||
| ); |
There was a problem hiding this comment.
hmm, if buffer is mmap file then i think array() will not work, afaik it only works for buffers that wrap a byte[]. Also is this log message displaying the buffer contents that useful? Why does this need to be in a try/catch at all?
This reverts commit 8b4a172.
|
As per discussion with Clint, instrumentation is not really helpful here. We can not print buffer as it can be really big (roughly speaking sometimes 100MB as well). Can you please +1 ? |
|
I really wasn’t suggesting to print the whole buffer. We know that the code think it needs to read 20 bytes while only 18 were left. So we can print those 18 bytes and can inspect that. One other thing we can also do is copy the offending sequence to a temporary location so it can be downloaded for offline inspection. |
4 participants
Description
This PR has potential fix for the following exception, found that we are creating immutable node and passing the mutable data buffer to immutable node.
This PR has: