Skip to content

Fix 2609 binary userdata lists#2785

Open
Dingyi189-eng wants to merge 2 commits into
jMonkeyEngine:masterfrom
Dingyi189-eng:fix-2609-binary-userdata-lists
Open

Fix 2609 binary userdata lists#2785
Dingyi189-eng wants to merge 2 commits into
jMonkeyEngine:masterfrom
Dingyi189-eng:fix-2609-binary-userdata-lists

Conversation

@Dingyi189-eng
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a unique identification system for UserData to ensure consistent serialization of list, map, and array fields, replacing the previous hardcoded indexing. It also enables list consistency testing in JmeExporterTest. A critical issue was identified in BinaryInputCapsule.java, where the logic added to readLong incorrectly handles special markers, potentially leading to stream corruption when reading specific long values.

Comment on lines +962 to +965
if (value == BinaryOutputCapsule.NULL_OBJECT
|| value == BinaryOutputCapsule.DEFAULT_OBJECT) {
index -= 4;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This change introduces a critical bug that will lead to stream corruption when reading long values of -1 or -2.

The index -= 4 logic is intended to compensate for the way BinaryOutputCapsule.deflate handles int values of -1 (NULL_OBJECT) or -2 (DEFAULT_OBJECT) by converting them into a single-byte marker. However, deflate only applies this special handling to 4-byte values (integers). For 8-byte long values, it writes the full (or partially deflated) bytes without using the magic markers.

Consequently, when readLong encounters a full 8-byte -1L, it will correctly increment the index by 9 bytes (1 length byte + 8 data bytes), but then this new code will incorrectly backtrack the index by 4 bytes. This causes the next read operation to start in the middle of the previous long value, corrupting the entire import process.

References
  1. Avoid using magic numbers (e.g., -1) as markers or default values in serialization to detect missing keys or special states, as this can lead to stream corruption or mask bugs.

Copy link
Copy Markdown

@xinzhoudev xinzhoudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Motivation
The BinaryExporter in jMonkeyEngine currently fails to correctly deserialize Savable objects that contain multiple UserData lists. When a scene object carries more than one such list, the binary import process misreads the stream, causing subsequent data to be parsed from the wrong offset. This leads to corrupted deserialization results and potential crashes, as reported in issue #2609.
The root cause lies in how BinaryInputCapsule handles the index pointer after reading certain values. The existing logic did not account for cases where multiple consecutive list structures are present, causing the read cursor to fall out of sync with the actual byte stream layout.

Summary
This PR makes two changes to address the issue:

  1. BinaryInputCapsule.java — Index correction on NULL_OBJECT / DEFAULT_OBJECT sentinel values
    When reading a long value that matches the NULL_OBJECT (-1) or DEFAULT_OBJECT (-2) sentinel constants defined in BinaryOutputCapsule, the index is now rewound by 4 bytes. This compensates for the fact that BinaryOutputCapsule.deflate() encodes these special int-sized markers in a compressed form, so the reader must not advance the full 8-byte stride in such cases.

⚠️ Note: A concern has been raised (by Gemini Code Assist) that this correction may itself introduce stream corruption when a genuine long value of -1L or -2L is written, since deflate() does not apply the same single-byte compression to 8-byte longs. This edge case should be validated before merge.

  1. JmeExporterTest.java — Enable list serialization tests
    The testLists flag in the exporter consistency test suite is changed from false to true, ensuring that list serialization and deserialization (including multiple UserData lists) is now covered by automated tests, preventing regression.

Copy link
Copy Markdown
Contributor

@JNightRider JNightRider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this PR contains the same changes as this PR - #2610, only this one slightly modifies the format of the prefix proposed by the previous pull request.

I'm not so sure about the other one, @riccardobl was there any reason to close that PR - #2610?

if (value == BinaryOutputCapsule.NULL_OBJECT
|| value == BinaryOutputCapsule.DEFAULT_OBJECT) {
index -= 4;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested these changes without this code snippet; the test runs without issues, so I suspect it's not related to the problem.


if (uniqueId == null) {
synchronized (this) {
if (uniqueId == null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the double validation; isn't one validation enough?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants