Fix OOB reads #3343
Merged
Fix OOB reads #3343
Conversation
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
254f1fb to
7725421
Compare
Member
|
Can you tell them to share the map that contains the event with us? (they can delete unrelated events before) Your fix probably correct (my guess is the Besides; Slowly fixing all commands they randomly find doesn't really scale... Needs something better. |
Member
|
Long term solution would be adding size checks to all commands... Small overhead in typical case (params have expected size): One size check per command + hint that the branch is usually not taken Helper function: diff --git a/src/game_interpreter_shared.h b/src/game_interpreter_shared.h
index 42c27990d..aa111efe1 100644
--- a/src/game_interpreter_shared.h
+++ b/src/game_interpreter_shared.h
@@ -23,6 +23,7 @@
#include <lcf/rpg/movecommand.h>
#include <lcf/rpg/saveeventexecframe.h>
#include <string_view.h>
+#include "compiler.h"
class Game_Character;
class Game_BaseInterpreterContext;
@@ -167,6 +168,25 @@ protected:
inline int ValueOrVariableBitfield(lcf::rpg::EventCommand const& com, int mode_idx, int shift, int val_idx) const {
return Game_Interpreter_Shared::ValueOrVariableBitfield<validate_patches, support_indirect_and_switch, support_scopes, support_named>(com, mode_idx, shift, val_idx, *this);
}
+
+ template<typename RetType, typename ClsType, typename... Args>
+ ClsType MemFnCls(RetType(ClsType::*)(Args...));
+
+ template<auto CMDFN, size_t MIN_SIZE>
+ bool CmdCall(lcf::rpg::EventCommand const& com) {
+ using ClassType = decltype(MemFnCls(CMDFN));
+
+ if (EP_UNLIKELY(com.parameters.size() < MIN_SIZE)) {
+ // Slow resizing of the parameters
+ // Only happens for malformed commands
+ auto ncom = com;
+ ncom.parameters = lcf::DBArray<int32_t>(MIN_SIZE);
+ std::copy(com.parameters.begin(), com.parameters.end(), ncom.parameters.begin());
+ return (static_cast<ClassType*>(this)->*CMDFN)(ncom);
+ }
+
+ return (static_cast<ClassType*>(this)->*CMDFN)(com);
+ }
};
#endifExample usage: diff --git a/src/game_interpreter.cpp b/src/game_interpreter.cpp
index 91821c45e..ccfd72214 100644
--- a/src/game_interpreter.cpp
+++ b/src/game_interpreter.cpp
@@ -582,9 +582,9 @@ bool Game_Interpreter::ExecuteCommand() {
bool Game_Interpreter::ExecuteCommand(lcf::rpg::EventCommand const& com) {
switch (static_cast<Cmd>(com.code)) {
case Cmd::ShowMessage:
- return CommandShowMessage(com);
+ return CmdCall<&Game_Interpreter::CommandShowMessage, 0>(com); // useless for 0
case Cmd::MessageOptions:
- return CommandMessageOptions(com);
+ return CmdCall<&Game_Interpreter::CommandMessageOptions, 4>(com);
case Cmd::ChangeFaceGraphic:
return CommandChangeFaceGraphic(com);
case Cmd::ShowChoice:
@@ -647,7 +647,7 @@ bool Game_Interpreter::ExecuteCommand(lcf::rpg::EventCommand const& com) {
case Cmd::ChangeHeroTitle:
return CommandChangeHeroTitle(com);
case Cmd::ChangeSpriteAssociation:
- return CommandChangeSpriteAssociation(com);
+ return CmdCall<&Game_Interpreter::CommandChangeSpriteAssociation, 3>(com); // Better fix for #3314
case Cmd::ChangeActorFace:
return CommandChangeActorFace(com);
case Cmd::ChangeVehicleGraphic:
@@ -2110,16 +2110,8 @@ bool Game_Interpreter::CommandChangeSpriteAssociation(lcf::rpg::EventCommand con
}
auto file = ToString(CommandStringOrVariableBitfield(com, 3, 1, 4));
-
- int idx = 0;
- if (com.parameters.size() > 1) {
- idx = ValueOrVariableBitfield(com, 3, 2, 1);
- }
-
- bool transparent = false;
- if (com.parameters.size() > 2) {
- transparent = com.parameters[2] != 0;
- }
+ int idx = ValueOrVariableBitfield(com, 3, 2, 1);
+ bool transparent = com.parameters[2] != 0;
actor->SetSprite(file, idx, transparent);
Main_Data::game_player->ResetGraphic();For most commands (there are exceptions ^^) bool Game_Interpreter::CommandChangeSpriteAssociation(lcf::rpg::EventCommand const& com) { // code 10630
int id = ValueOrVariableBitfield(com, 3, 0, 0); // 0
Game_Actor* actor = Main_Data::game_actors->GetActor(id);
if (!actor) {
Output::Warning("ChangeSpriteAssociation: Invalid actor ID {}", id);
return true;
}
auto file = ToString(CommandStringOrVariableBitfield(com, 3, 1, 4));
int idx = ValueOrVariableBitfield(com, 3, 2, 1); // 1
bool transparent = com.parameters[2] != 0; // 2 -> HIGHEST, MIN_SIZE is 2+1=3
actor->SetSprite(file, idx, transparent);
Main_Data::game_player->ResetGraphic();
return true;
} |
Contributor
Author
|
I like your LTS solution better, maybe we can update like this as the errors popup |
7725421 to
82214fc
Compare
Contributor
Author
|
renamed it to |
Ghabry
reviewed
Jan 28, 2025
src/game_interpreter.cpp
Outdated
| transparent = com.parameters[2] != 0; | ||
| } | ||
| int idx = ValueOrVariableBitfield(com, 3, 2, 1); // 1 | ||
| bool transparent = com.parameters[2] != 0; // 2 -> HIGHEST, MIN_SIZE is 2+1=3 |
Member
There was a problem hiding this comment.
can you remove my // 0, 1, 2 example comments? 😅
Ghabry
reviewed
Jan 28, 2025
src/game_interpreter.cpp
Outdated
| return CmdSetup<&Game_Interpreter::CommandChangeSpriteAssociation, 3>(com); // FIX OOB access | ||
| case Cmd::ChangeActorFace: | ||
| return CommandChangeActorFace(com); | ||
| return CmdSetup<&Game_Interpreter::CommandChangeActorFace, 2>(com); // FIX OOB access |
Member
There was a problem hiding this comment.
Don't think the "Fix OOB access" comment is necessary, this function call will be slowly added to all commands.
Using Ghabry's proposed solution. Fixes an Issue similar to EasyRPG#3320
82214fc to
44f8b09
Compare
Contributor
Author
|
The MIN_SIZE on setface command may be 4, due to |
Member
|
The index used for String Variables are a Maniac extension so the function checks for both the mode and the value whether it would read out of bounds. Take a look at the vanilla command written by an unmodified version of the editor (not Maniac Patched, not TPCed, etc.). Look at the raw data of Map0004 in our test game (Show Face). It always has 3 parameters. <event_commands>
<EventCommand>
<code>10130</code>
<indent>0</indent>
<string>Chara1</string>
<parameters>1 0 0</parameters>
</EventCommand>
<EventCommand>
<code>10110</code>
<indent>0</indent>
<string>Showing a Face (this message has face)</string>
<parameters></parameters>
</EventCommand>
<EventCommand>
<code>10110</code>
<indent>0</indent>
<string>Testing continuiting of the face</string>
<parameters></parameters>
</EventCommand>
<EventCommand>
<code>20110</code>
<indent>0</indent>
<string>(this message has face too)</string>
<parameters></parameters>
</EventCommand>
<EventCommand>
<code>10130</code>
<indent>0</indent>
<string>Chara1</string>
<parameters>0 0 0</parameters>
</EventCommand>
<EventCommand>
<code>10110</code>
<indent>0</indent>
<string>Changing the face</string>
<parameters></parameters>
</EventCommand>
<EventCommand>
<code>20110</code>
<indent>0</indent>
<string>(The face changed)</string>
<parameters></parameters>
</EventCommand>
<EventCommand>
<code>10130</code>
<indent>0</indent>
<string></string>
<parameters>0 0 0</parameters>
</EventCommand>Now look at your CSA dump where its totally inconsitent |
Member
|
CSA vs. testgame is nice for comparison btw. Message Options is also a wild mix of 1 - 9 vs 4 in Vanilla. |
Contributor
Author
|
@Ghabry , Here's a map containing the problematic commands from the first print: |
Member
|
When I run this map using the current upstream Player I get the warning that "FaceSet/minnatsuki_01" was not found (because I don't have that asset) but this looks normal to me? So I don't get "Kki_001". |
Contributor
Author
|
I also could'nt reproduce the error. |
Member
|
added now size checks to all the event commands. Made it a draft because I want to execute everything in the testgame to see if the code path triggers because the min size is incorrect... |
Member
|
Script (requires fish and ruby) Prints for i in *.lmu;lcf2xml $i;end
for i in *.emu;cat $i >> cmds.xml;end
cat cmds.xml | ruby cmdcheck.rbcodes_max = {}
codes_min = {}
while gets
line = $_.chomp
if line.include? "<code>"
line = line.scan(/\<code\>(\d+)/)
cmd = line[0][0].to_i
puts line
gets
gets
gets
line = $_.chomp
#puts line
puts line.scan(/[> ](\d+)/).length
len = line.scan(/[> ](-?\d+)/).length
if codes_max.include? cmd
if codes_max[cmd] < len
codes_max[cmd] = len
end
if codes_min[cmd] > len
codes_min[cmd] = len
end
else
codes_max[cmd] = len
codes_min[cmd] = len
end
end
end
lookup = {
10 => "END",
1005 => "CallCommonEvent",
1006 => "ForceFlee",
1007 => "EnableCombo",
1008 => "ChangeClass",
1009 => "ChangeBattleCommands",
5001 => "OpenLoadMenu",
5002 => "ExitGame",
5003 => "ToggleAtbMode",
5004 => "ToggleFullscreen",
5005 => "OpenVideoOptions",
10110 => "ShowMessage",
10120 => "MessageOptions",
10130 => "ChangeFaceGraphic",
10140 => "ShowChoice",
10150 => "InputNumber",
10210 => "ControlSwitches",
10220 => "ControlVars",
10230 => "TimerOperation",
10310 => "ChangeGold",
10320 => "ChangeItems",
10330 => "ChangePartyMembers",
10410 => "ChangeExp",
10420 => "ChangeLevel",
10430 => "ChangeParameters",
10440 => "ChangeSkills",
10450 => "ChangeEquipment",
10460 => "ChangeHP",
10470 => "ChangeSP",
10480 => "ChangeCondition",
10490 => "FullHeal",
10500 => "SimulatedAttack",
10610 => "ChangeHeroName",
10620 => "ChangeHeroTitle",
10630 => "ChangeSpriteAssociation",
10640 => "ChangeActorFace",
10650 => "ChangeVehicleGraphic",
10660 => "ChangeSystemBGM",
10670 => "ChangeSystemSFX",
10680 => "ChangeSystemGraphics",
10690 => "ChangeScreenTransitions",
10710 => "EnemyEncounter",
10720 => "OpenShop",
10730 => "ShowInn",
10740 => "EnterHeroName",
10810 => "Teleport",
10820 => "MemorizeLocation",
10830 => "RecallToLocation",
10840 => "EnterExitVehicle",
10850 => "SetVehicleLocation",
10860 => "ChangeEventLocation",
10870 => "TradeEventLocations",
10910 => "StoreTerrainID",
10920 => "StoreEventID",
11010 => "EraseScreen",
11020 => "ShowScreen",
11030 => "TintScreen",
11040 => "FlashScreen",
11050 => "ShakeScreen",
11060 => "PanScreen",
11070 => "WeatherEffects",
11110 => "ShowPicture",
11120 => "MovePicture",
11130 => "ErasePicture",
11210 => "ShowBattleAnimation",
11310 => "PlayerVisibility",
11320 => "FlashSprite",
11330 => "MoveEvent",
11340 => "ProceedWithMovement",
11350 => "HaltAllMovement",
11410 => "Wait",
11510 => "PlayBGM",
11520 => "FadeOutBGM",
11530 => "MemorizeBGM",
11540 => "PlayMemorizedBGM",
11550 => "PlaySound",
11560 => "PlayMovie",
11610 => "KeyInputProc",
11710 => "ChangeMapTileset",
11720 => "ChangePBG",
11740 => "ChangeEncounterSteps",
11750 => "TileSubstitution",
11810 => "TeleportTargets",
11820 => "ChangeTeleportAccess",
11830 => "EscapeTarget",
11840 => "ChangeEscapeAccess",
11910 => "OpenSaveMenu",
11930 => "ChangeSaveAccess",
11950 => "OpenMainMenu",
11960 => "ChangeMainMenuAccess",
12010 => "ConditionalBranch",
12110 => "Label",
12120 => "JumpToLabel",
12210 => "Loop",
12220 => "BreakLoop",
12310 => "EndEventProcessing",
12320 => "EraseEvent",
12330 => "CallEvent",
12410 => "Comment",
12420 => "GameOver",
12510 => "ReturntoTitleScreen",
13110 => "ChangeMonsterHP",
13120 => "ChangeMonsterMP",
13130 => "ChangeMonsterCondition",
13150 => "ShowHiddenMonster",
13210 => "ChangeBattleBG",
13260 => "ShowBattleAnimation_B",
13310 => "ConditionalBranch_B",
13410 => "TerminateBattle",
20110 => "ShowMessage_2",
20140 => "ShowChoiceOption",
20141 => "ShowChoiceEnd",
20710 => "VictoryHandler",
20711 => "EscapeHandler",
20712 => "DefeatHandler",
20713 => "EndBattle",
20720 => "Transaction",
20721 => "NoTransaction",
20722 => "EndShop",
20730 => "Stay",
20731 => "NoStay",
20732 => "EndInn",
22010 => "ElseBranch",
22011 => "EndBranch",
22210 => "EndLoop",
22410 => "Comment_2",
23310 => "ElseBranch_B",
23311 => "EndBranch_B",
2002 => "EasyRpg_TriggerEventAt",
2003 => "EasyRpg_SmartMoveRoute",
2004 => "EasyRpg_SmartStepToward",
2050 => "EasyRpg_CallMovementAction",
2051 => "EasyRpg_WaitForSingleMovement",
2052 => "EasyRpg_AnimateVariable",
2053 => "EasyRpg_SetInterpreterFlag",
2055 => "EasyRpg_ProcessJson",
2056 => "EasyRpg_CloneMapEvent",
2057 => "EasyRpg_DestroyMapEvent",
2058 => "EasyRpg_StringPictureMenu",
3001 => "Maniac_GetSaveInfo",
3002 => "Maniac_Save",
3003 => "Maniac_Load",
3004 => "Maniac_EndLoadProcess",
3005 => "Maniac_GetMousePosition",
3006 => "Maniac_SetMousePosition",
3007 => "Maniac_ShowStringPicture",
3008 => "Maniac_GetPictureInfo",
3009 => "Maniac_ControlBattle",
3010 => "Maniac_ControlAtbGauge",
3011 => "Maniac_ChangeBattleCommandEx",
3012 => "Maniac_GetBattleInfo",
3013 => "Maniac_ControlVarArray",
3014 => "Maniac_KeyInputProcEx",
3015 => "Maniac_RewriteMap",
3016 => "Maniac_ControlGlobalSave",
3017 => "Maniac_ChangePictureId",
3018 => "Maniac_SetGameOption",
3019 => "Maniac_CallCommand",
3020 => "Maniac_ControlStrings",
3021 => "Maniac_GetGameInfo",
3025 => "Maniac_EditPicture",
3026 => "Maniac_WritePicture",
3027 => "Maniac_AddMoveRoute",
3028 => "Maniac_EditTile",
}
plookup = {
"ShowMessage" => 0,
"MessageOptions" => 4,
"ChangeFaceGraphic" => 3,
"ShowChoice" => 1,
"ShowChoiceOption" => 1,
"ShowChoiceEnd" => 0,
"InputNumber" => 2,
"ControlSwitches" => 4,
"ControlVars" => 7,
"TimerOperation" => 5,
"ChangeGold" => 3,
"ChangeItems" => 5,
"ChangePartyMembers" => 3,
"ChangeExp" => 6,
"ChangeLevel" => 6,
"ChangeParameters" => 6,
"ChangeSkills" => 5,
"ChangeEquipment" => 5,
"ChangeHP" => 6,
"ChangeSP" => 5,
"ChangeCondition" => 4,
"FullHeal" => 2,
"SimulatedAttack" => 8,
"Wait" => 1,
"PlayBGM" => 4,
"FadeOutBGM" => 1,
"PlaySound" => 3,
"EndEventProcessing" => 0,
"Comment:Comment_2" => 0,
"GameOver" => 0,
"ChangeHeroName" => 1,
"ChangeHeroTitle" => 1,
"ChangeSpriteAssociation" => 3,
"ChangeActorFace" => 2,
"ChangeVehicleGraphic" => 2,
"ChangeSystemBGM" => 5,
"ChangeSystemSFX" => 4,
"ChangeSystemGraphics" => 2,
"ChangeScreenTransitions" => 2,
"MemorizeLocation" => 3,
"SetVehicleLocation" => 5,
"ChangeEventLocation" => 4,
"TradeEventLocations" => 2,
"StoreTerrainID" => 4,
"StoreEventID" => 4,
"EraseScreen" => 1,
"ShowScreen" => 1,
"TintScreen" => 6,
"FlashScreen" => 6,
"ShakeScreen" => 4,
"WeatherEffects" => 2,
"ShowPicture" => 14,
"MovePicture" => 16,
"ErasePicture" => 1,
"PlayerVisibility" => 1,
"MoveEvent" => 4,
"MemorizeBGM" => 0,
"PlayMemorizedBGM" => 0,
"KeyInputProc" => 5,
"ChangeMapTileset" => 1,
"ChangePBG" => 6,
"ChangeEncounterSteps" => 1,
"TileSubstitution" => 3,
"TeleportTargets" => 6,
"ChangeTeleportAccess" => 1,
"EscapeTarget" => 5,
"ChangeEscapeAccess" => 1,
"ChangeSaveAccess" => 1,
"ChangeMainMenuAccess" => 1,
"ConditionalBranch" => 6,
"JumpToLabel" => 1,
"Loop" => 0,
"BreakLoop" => 0,
"EndLoop" => 0,
"EraseEvent" => 0,
"CallEvent" => 3,
"ReturntoTitleScreen" => 0,
"ChangeClass" => 7,
"ChangeBattleCommands" => 4,
"ElseBranch" => 0,
"EndBranch" => 0,
"ExitGame" => 0,
"ToggleFullscreen" => 0,
"OpenVideoOptions" => 0,
"Maniac_GetSaveInfo" => 12,
"Maniac_Load" => 4,
"Maniac_Save" => 3,
"Maniac_EndLoadProcess" => 0,
"Maniac_GetMousePosition" => 2,
"Maniac_SetMousePosition" => 3,
"Maniac_ShowStringPicture" => 23,
"Maniac_GetPictureInfo" => 8,
"Maniac_ControlVarArray" => 5,
"Maniac_KeyInputProcEx" => 4,
"Maniac_RewriteMap" => 9,
"Maniac_ControlGlobalSave" => 6,
"Maniac_ChangePictureId" => 6,
"Maniac_SetGameOption" => 4,
"Maniac_ControlStrings" => 8,
"Maniac_CallCommand" => 6,
"Maniac_GetGameInfo" => 8,
"EasyRpg_SetInterpreterFlag" => 2,
"EasyRpg_ProcessJson" => 8,
"EasyRpg_CloneMapEvent" => 10,
"EasyRpg_DestroyMapEvent" => 2,
"RecallToLocation" => 3,
"EnemyEncounter" => 6,
"VictoryHandler" => 0,
"EscapeHandler" => 0,
"DefeatHandler" => 0,
"EndBattle" => 0,
"OpenShop" => 4,
"Transaction" => 0,
"NoTransaction" => 0,
"EndShop" => 0,
"ShowInn" => 3,
"Stay" => 0,
"NoStay" => 0,
"EndInn" => 0,
"EnterHeroName" => 3,
"Teleport" => 3,
"EnterExitVehicle" => 0,
"PanScreen" => 5,
"ShowBattleAnimation" => 4,
"FlashSprite" => 7,
"ProceedWithMovement" => 0,
"HaltAllMovement" => 0,
"PlayMovie" => 5,
"OpenSaveMenu" => 0,
"OpenMainMenu" => 0,
"OpenLoadMenu" => 0,
"ToggleAtbMode" => 0,
"EasyRpg_TriggerEventAt" => 4,
"EasyRpg_WaitForSingleMovement" => 6,
"CallCommonEvent" => 1,
"ForceFlee" => 3,
"EnableCombo" => 3,
"ChangeMonsterHP" => 5,
"ChangeMonsterMP" => 4,
"ChangeMonsterCondition" => 3,
"ShowHiddenMonster" => 1,
"ChangeBattleBG" => 0,
"ShowBattleAnimation_B" => 3,
"TerminateBattle" => 0,
"ConditionalBranch_B" => 5,
"ElseBranch_B" => 0,
"EndBranch_B" => 0,
"Maniac_ControlBattle" => 4,
"Maniac_ControlAtbGauge" => 7,
"Maniac_ChangeBattleCommandEx" => 2,
"Maniac_GetBattleInfo" => 5
}
puts "x"
codes_min.each { |k, v|
cmax = codes_max[k]
p = plookup[lookup[k]]
puts("#{lookup[k]}: #{p} (#{v}/#{cmax})")
if p.nil?
puts("nil!")
elsif v < p
puts("!!!!!!!")
end
} |
Member
|
Checked some games. Here some suspicious that must be tested: Output is "Required MIN (Game Min/Game Max)" Violated Heroine: EDIT: False positive. Their are some control characters in the messages (???) which mess up my parsing. Current online version of CU: |
Also fixed a oob access in ShowBattleAnimation and OpenSaveMenu
Member
|
Okay I think this is ready. The ones in VH1 were a bug in my ruby script because of weird control characters in the messages o_O CU was created partially via Maniac Patch TPC I guess which takes some shortcuts. Empty Wait is likely "Wait 0" (1 frame wait) and not all Control Vars require that many arguments. So looks okay. Bonus: CSA (Cold Spaghetti Analyzer) edb file to show what TPC is doing. These out of bounds bugs were fixed by this PR: |
fdelapena
approved these changes
Feb 24, 2025
Contributor
Author
|
More annoying side effects of maniacs commands: This "Apply Scalling to Determine Position" usually comes auto checked in maniacs newer menus: In easyRPG it's being read as "wait for completion" and it is creating unwanted side effects. |
Member
|
But this is because they redefined a single bool into a list of flags. |
Contributor
Author
|
ugh, still some problems with OOB entries You can see in this game. Compare "Parallax A" with "Problematic Parallax A" while walking around after chosing one of those. What is breaking is this flag that starts auto toggled inside |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Fixes an Issue similar to #3320
Wasn't able to reproduce it, but here are some complains about this issue from other users:
Maniacs new editor menus seems to be creating annoying OOB command patterns in different places...