You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(motchan): Prevent undefined behavior when reading floats from motion channel packets - #3648
Follow-up to #3630, #3635 and #3636. TimeCodedMotionChannelClass and AdaptiveDeltaMotionChannelClass store their animation packets in a uint32 array and read the float values through float* casts. Reading a uint32 object as a float is undefined behavior, the same problem as #3521.
The motion channel code in both games now reads each uint32 element and converts it with std::bit_cast<float32>:
TimeCodedMotionChannelClass::Get_Vector: for the copied and the interpolated vector values.
TimeCodedMotionChannelClass::Get_QuatVector: a new Set_Quaternion_From_Packet helper sets a quaternion from the four floats of a packet. It replaces the four q.Set calls that read through a float*.
AdaptiveDeltaMotionChannelClass::decompress: for the starting value of each vector component. The other decompress overload only used its float* to find where the packet bytes start, so it now points a const uint32* there.
The Generals and Zero Hour files are identical since #3594, and both get the same change.
Evidence
Disassembly: I compared both games' motchan objects against a build of main.
VS 2026 (ClangCL) Release: every function is identical.
VC6 Release: only Get_Vector, Get_QuatVector and the first decompress overload changed. VC6 now copies the packet values with integer moves where it used fld/fstp before. The float arithmetic instructions and the calls in each function are the same as on main.
AI Disclosure
Claude Opus 5.5 was used to aid in implementation and to compare the disassembly.
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Walkthrough
Both game variants update motion-channel packet reads to use std::bit_cast or memcpy instead of float-pointer casts. The changes cover vector and quaternion retrieval, interpolation, and adaptive-delta decompression.
Changes
Motion Packet Reads
Layer / File(s)
Summary
Vector and quaternion packet reads Generals/Code/Libraries/Source/WWVegas/WW3D2/motchan.cpp, GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/motchan.cpp
Vector and quaternion packet values use std::bit_cast instead of float-pointer reinterpretation. Packet-copy paths use memcpy. A local helper converts four packet values to a quaternion.
The decompressor uses uint32 pointers for packed initial values and bit-casts the initial value to float. The continuation decompressor uses an offset into the uint32 data.
The packet-read changes have no identified merge-blocking issue and appear mergeable subject to normal build and test checks.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: a fix that prevents undefined behavior when reading floats from motion channel packets. The changeset replaces float* reads from uint32 packet arrays with std::bit_cas…
Description check
The description is directly related to the changeset. It explains that TimeCodedMotionChannelClass and AdaptiveDeltaMotionChannelClass read float values through float* casts from uint32 arrays. It lis…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main change: a fix that prevents undefined behavior when reading floats from motion channel packets. The changeset replaces float* reads from uint32 packet arrays with std::bit_cast conversions in motchan.cpp for both Generals and Zero Hour, which matches the title. The title uses a conventional prefix and is slightly long at 88 characters, but it is specific and accurate.
Full details: Description check
Explanation
The description is directly related to the changeset. It explains that TimeCodedMotionChannelClass and AdaptiveDeltaMotionChannelClass read float values through float* casts from uint32 arrays. It lists the changed functions (Get_Vector, Get_QuatVector with the new Set_Quaternion_From_Packet helper, and decompress), states that both games receive the same change, and gives disassembly evidence. It is detailed and on-topic.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
TimeCodedMotionChannelClass and AdaptiveDeltaMotionChannelClass store
their packets in uint32 arrays and read the float values through float
pointers. Read each uint32 element and convert it with std::bit_cast.
AdaptiveDeltaMotionChannelClass::decompress only used its float pointer
to find where the packet bytes start, so it now points a uint32 pointer
there instead.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FixIs fixing something, but is not user facingMinorSeverity: Minor < Major < Critical < Blocker
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
xezon
added
Minor
Follow-up to #3630, #3635 and #3636.
TimeCodedMotionChannelClassandAdaptiveDeltaMotionChannelClassstore their animation packets in auint32array and read the float values throughfloat*casts. Reading auint32object as afloatis undefined behavior, the same problem as #3521.The motion channel code in both games now reads each
uint32element and converts it withstd::bit_cast<float32>:TimeCodedMotionChannelClass::Get_Vector: for the copied and the interpolated vector values.TimeCodedMotionChannelClass::Get_QuatVector: a newSet_Quaternion_From_Packethelper sets a quaternion from the four floats of a packet. It replaces the fourq.Setcalls that read through afloat*.AdaptiveDeltaMotionChannelClass::decompress: for the starting value of each vector component. The otherdecompressoverload only used itsfloat*to find where the packet bytes start, so it now points aconst uint32*there.The Generals and Zero Hour files are identical since #3594, and both get the same change.
Evidence
motchanobjects against a build ofmain.Get_Vector,Get_QuatVectorand the firstdecompressoverload changed. VC6 now copies the packet values with integer moves where it usedfld/fstpbefore. The float arithmetic instructions and the calls in each function are the same as onmain.AI Disclosure
Claude Opus 5.5 was used to aid in implementation and to compare the disassembly.