Fixed UnpackerConfig.bufferSize to work #658
Conversation
| @@ -518,7 +518,7 @@ public boolean isStr8FormatSupport() | |||
|
|
|||
| private int stringSizeLimit = Integer.MAX_VALUE; | |||
|
|
|||
| private int bufferSize = 8192; | |||
| private int bufferSize = 100 * 1024 * 1024; | |||
There was a problem hiding this comment.
Don't change the default buffer size. msgpack-java is frequently used even for the small size of data.
There was a problem hiding this comment.
Especially, creating a large buffer is not free as Java will use CPU resources for filling zeros to the array contents.
| * @throws IOException when underlying input throws IOException | ||
| */ | ||
| public byte[] readPayload(int length) | ||
| throws IOException | ||
| { | ||
| if (length > bufferSize) { |
There was a problem hiding this comment.
This part is a good catch. If that happens, instead of throwing an error, we need to enlarge the buffer size by creating a new buffer large enough for storing the expected data size.
This implementation would be similar to ArrayList implementation of Java.
There was a problem hiding this comment.
I'm sorry that I don't quite understand here.
Am I correct that the purpose of UnpackerConfig.bufferSize is to limit the size of the new byte array in readPayload(int)?
Do I need to change this code?
There was a problem hiding this comment.
- Do not change the default buffer size from 8K
- Setting the default buffer size from config is ok
- Do not throw an exception in unpackXXXheader. Only readPayload needs to be fixed to support large volumes of data
- readPayload should swap the existing buffer to a large buffer if the payload length exceeds the current bufferSize
| int len; | ||
| if (Code.isFixedArray(b)) { // fixarray | ||
| return b & 0x0f; | ||
| len = b & 0x0f; | ||
| } | ||
| switch (b) { | ||
| case Code.ARRAY16: { // array 16 | ||
| int len = readNextLength16(); | ||
| return len; | ||
| } | ||
| case Code.ARRAY32: { // array 32 | ||
| int len = readNextLength32(); | ||
| return len; | ||
| else { | ||
| switch (b) { | ||
| case Code.ARRAY16: { // array 16 | ||
| len = readNextLength16(); | ||
| break; | ||
| } | ||
| case Code.ARRAY32: { // array 32 | ||
| len = readNextLength32(); | ||
| break; | ||
| } | ||
| default: { | ||
| throw unexpected("Array", b); | ||
| } | ||
| } | ||
| } | ||
| throw unexpected("Array", b); | ||
|
|
||
| if (len > bufferSize) { | ||
| throw new MessageSizeException(String.format("cannot unpack a Array of size larger than %,d: %,d", bufferSize, len), len); | ||
| } | ||
|
|
||
| return len; |
There was a problem hiding this comment.
Throwing error here is unnecessary if readPayload checks the buffer size.
| int len; | ||
| if (Code.isFixedMap(b)) { // fixmap | ||
| return b & 0x0f; | ||
| } | ||
| switch (b) { | ||
| case Code.MAP16: { // map 16 | ||
| int len = readNextLength16(); | ||
| return len; | ||
| } | ||
| case Code.MAP32: { // map 32 | ||
| int len = readNextLength32(); | ||
| return len; | ||
| else { | ||
| switch (b) { | ||
| case Code.MAP16: { // map 16 | ||
| len = readNextLength16(); | ||
| break; | ||
| } | ||
| case Code.MAP32: { // map 32 | ||
| len = readNextLength32(); | ||
| break; | ||
| } | ||
| default: { | ||
| throw unexpected("Map", b); | ||
| } | ||
| } | ||
| } | ||
| throw unexpected("Map", b); | ||
|
|
||
| if (len > bufferSize / 2) { | ||
| throw new MessageSizeException(String.format("cannot unpack a Map of size larger than %,d: %,d", bufferSize / 2, len), len); | ||
| } | ||
|
|
||
| return len; |
There was a problem hiding this comment.
Unnecessary change for the same reason
|
Buffer size limits now work, but some tests no longer pass. |
|
Upon re-reading the PR, I noticed some misunderstandings. UnpackerConfig.bufferSize is used to control the internal buffer size, but it is not meant to limit the size of the uncompressed data. In order to prevent loading excessively large data into memory, we need to introduce another configuration. I will close this pull request because it has become outdated. The requirement to limit the uncompressed data size and fail the Unpacker is still valid, but it should be addressed in a different context. |
This will Resolve #657
It limits internal buffer size when unpacking Array and Map.
However, I think the default bufferSize of 8192 is too small.
I changed the default buffer size to 100MiB following msgpack-python.