-
Notifications
You must be signed in to change notification settings - Fork 151
bugfix(projectile): Fix out-of-bounds access in DumbProjectile which causes mismatch with very high speed weapons at small hit distances #2087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks logical in principle.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks correct to me.
|
I hadn't at looked the Generals code until now. Lines 651 to 652 in 8c560e6
Lines 635 to 638 in 8c560e6
It doesn't have this particular OOB bug, but I'm not entirely sure why the code was updated for Zero Hour or how the behavior changed. |
|
Maybe we can just leave the Generals code 'as is', and look at it in more detail later as part of a unification effort for this code. |
|
Is the code very different yes? |
|
Check the links two comments back. It's not that different, but it doesn't have this particular OOB bug. |
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR fixes a critical out-of-bounds memory access bug in Root Cause: When a weapon has sufficiently high speed, the flight path Bezier curve calculation produces fewer than 2 points in The Fix:
Impact: Eliminates mismatch issues for patched clients. Retail mode still warns about potential mismatches with unpatched clients, but behavior is now deterministic for patched clients. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Game as Game Logic
participant DPB as DumbProjectileBehavior
participant FlightPath as m_flightPath Vector
participant Obj as Projectile Object
Note over Game,Obj: High-speed weapon triggers projectile update
Game->>DPB: update()
DPB->>DPB: Check lifespan
DPB->>FlightPath: Check m_currentFlightPathStep >= size()
alt Flight path exhausted
DPB->>DPB: detonate()
DPB-->>Game: UPDATE_SLEEP_NONE
else Valid flight step
DPB->>FlightPath: flightStep = m_flightPath[m_currentFlightPathStep]
alt orientToFlightPath enabled
alt m_currentFlightPathStep > 0
DPB->>FlightPath: Get prevPos from [step-1]
DPB->>DPB: Calculate orientation
else step == 0 (Bug Fix Path)
DPB->>FlightPath: Check size() >= 2
alt size >= 2 (Normal case)
DPB->>FlightPath: prevPos = m_flightPath[0]
DPB->>FlightPath: curPos = m_flightPath[1]
else size < 2 (Bug case - FIXED)
Note over DPB: BEFORE: Out-of-bounds access!<br/>AFTER: Use fallback values
DPB->>DPB: flightStep = m_flightPathStart
DPB->>DPB: prevPos = m_flightPathStart
DPB->>DPB: curPos = m_flightPathEnd
alt RETAIL_COMPATIBLE_CRC
DPB->>DPB: DEBUG_CRASH warning about mismatch
end
end
DPB->>DPB: Calculate curDir and orientation
DPB->>Obj: setTransformMatrix(orientMtx)
end
else no orientation
DPB->>Obj: setPosition(flightStep)
end
DPB->>DPB: Increment m_currentFlightPathStep
DPB-->>Game: UPDATE_SLEEP_NONE
end
|
Dismissing Approval because this needs one more looking regarding m_flightPathSegments ==1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
Moved code comment.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
Show resolved
Hide resolved
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Should be good then.
|
I'll put my last comment here, so it doesn't get buried. With fewer than 2 points, there's no visible projectile, but that's to be expected. There's also no bullet impact when force firing on the ground. Where the damage is applied needs more testing, because it seems like it's applied to a much wider field. |
Sufficiently high weapon speeds expose an out-of-bounds access bug for
DumbProjectile, which is the reason for non-deterministic behavior across clients (i.e. mismatches).This PR fixes the bug properly in no retail mode, and makes behavior deterministic for patched clients in retail mode.
TODO:
[ ] Replicate in GeneralsGenerals doesn't have this particular out-of-bounds bug.