Skip to content

Fix map size overflow in unpackValue - #1044

Open
PHJ2000 wants to merge 2 commits into
msgpack:mainfrom
PHJ2000:fix/map-value-size-overflow
Open

PHJ2000 wants to merge 2 commits into
msgpack:mainfrom
PHJ2000:fix/map-value-size-overflow

Conversation

@PHJ2000

@PHJ2000 PHJ2000 commented Nov 6, 2026

Copy link
Copy Markdown

Fixes #1042.

Check the map size before allocating the interleaved key/value array in both MessageUnpacker.unpackValue() overloads. Pair counts from 2^30 through 2^31 - 1 now throw the existing MessageSizeException, with the declared count available through getSize(), instead of overflowing size * 2 and throwing NegativeArraySizeException. Larger unsigned headers retain their existing size exception.

The streaming unpackMapHeader() range is preserved. This change addresses the integer overflow during value materialization; it does not introduce a general memory limit or gradual allocation for #657.

Validation

The same 21 new AirSpec cases produced four expected errors before the fix and passed afterward; the other 17 cases passed on both versions. They cover both overloads, the overflow boundary, existing unsigned-size rejection, ordinary fixmap/map16/map32 values, nested maps, and streaming headers.

All 21 cases pass with both default and universal buffers on actual Java 8, 11, 17, 21 and 24 test JVMs. scalafmtCheckAll and jcheckStyle pass.

The existing pending entry is the malformed UTF-8 string case in MessagePackTest.

Broader local results:

Check Result
Java 8 core, default and universal Each reports 214 passed entries and one existing pending entry, after compiling against the Java 8 API surface.
Java 21 core, default A full run reports 214 passed entries and the same pending entry.
Java 21 core, universal Base: 209 passed, five errors, one pending. Patched: 213 passed, one error, one pending. The four removed errors are the new map regressions.
Java 21 Jackson, explicit JUnit Platform execution Base and patched versions both have 50 passed and the same 27 failures across 77 entries.

The remaining universal Java 21 error is the unchanged DirectBufferAccessTest, which accesses MessageBuffer.unsafe when universal mode leaves it null. Of the 27 unchanged Jackson failures, 26 are null-pointer failures associated with the existing mixture of Jupiter test methods and JUnit 4 fixture lifecycle annotations. The remaining failure is testMessagePackParserDirectly(): its parser-location assertion expects 0 and receives -1, independently of those fixtures. These limitations were also documented in #1037 and were rechecked for this change. The stock sbt task does not discover the Jackson JUnit tests, so its zero-test result is not counted as a successful Jackson run.

AirSpec’s stock XML repeats leaf names for some nested tests. Additional full core runs used a verification-only copy of its event reporter that includes the actual parent test context in each name. Those reports contain 215 distinct testcase names and preserve the outcomes above; assertions, test execution, status and errors were unchanged. The 21 new regression cases and the focused Java-version matrix also passed with the unmodified reporter. This is not a claim that every project test is green. Remote CI has not run for this unpublished branch.

Portable reproduction commands and build notes

Run sbt itself on JDK 21. Set TEST_JAVA_HOME to the JDK whose compiler/test runtime is being checked. The local evidence additionally recorded the forked JVM identity; that diagnostic did not transform classes or change test behavior.

TEST_JAVA_HOME=/path/to/jdk21 ./sbt -batch \
  'msgpack-core/testOnly org.msgpack.core.MapSizeLimitTest'
./sbt -batch scalafmtCheckAll jcheckStyle

For the core suite, use testOnly * to select every class explicitly; sbt 2's test uses testQuick and may reuse previous results:

TEST_JAVA_HOME=/path/to/jdk21 ./sbt -batch \
  'msgpack-core/testOnly *'
TEST_JAVA_HOME=/path/to/jdk21 ./sbt -batch \
  'set msgpackCore / Test / javaOptions += "-Dmsgpack.universal-buffer=true"' \
  'msgpack-core/testOnly *'

The initial Java 8 attempt encountered NoSuchMethodError from classes linked against newer covariant NIO methods. The exact origin of that bytecode or cache reuse was not established. An explicit session setting followed by recompilation recovered the Java 8 runs:

TEST_JAVA_HOME=/path/to/jdk8 ./sbt -batch \
  'set msgpackCore / Compile / javacOptions ++= Seq("-bootclasspath", "/path/to/jdk8/jre/lib/rt.jar")' \
  'msgpack-core/testOnly *'
TEST_JAVA_HOME=/path/to/jdk8 ./sbt -batch \
  'set msgpackCore / Compile / javacOptions ++= Seq("-bootclasspath", "/path/to/jdk8/jre/lib/rt.jar")' \
  'set msgpackCore / Test / javaOptions += "-Dmsgpack.universal-buffer=true"' \
  'msgpack-core/testOnly *'

For Jackson, obtain its Test/fullClasspath from sbt and run JUnit Platform Console Standalone 1.14.4. The normal class-name scan covers 76 entries; the additional example class contributes one entry:

java -jar junit-platform-console-standalone-1.14.4.jar execute \
  --class-path '<msgpack-jackson Test/fullClasspath>' \
  --scan-class-path --include-package org.msgpack.jackson.dataformat \
  --fail-if-no-tests --details summary
java -jar junit-platform-console-standalone-1.14.4.jar execute \
  --class-path '<msgpack-jackson Test/fullClasspath>' \
  --select-class org.msgpack.jackson.dataformat.ExampleOfTypeInformationSerDe \
  --fail-if-no-tests --details summary

The report-only verification input comes from AirSpec 2026.2.4. Its sole change qualifies the emitted event name with parentContext.testName after the test has completed. It was supplied through a local sbt session setting, is outside this patch, and does not change test selection or expectations. The original reports and this diagnostic source are retained separately.

The fix and tests are limited to MessageUnpacker and the new MapSizeLimitTest. Build settings and the unrelated fixtures remain outside this patch. #1026 also touches MessageUnpacker; its current depth-limit change leaves these two overflowing allocations in place.

@github-actions github-actions Bot added the bug label Nov 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MessageUnpacker#unpackValue can cause NegativeArraySizeException for large maps

1 participant