Add sanity checks to all LCF data structure access #1259

Ghabry · 2017-09-09T11:33:55Z

to reduce crashes and exploit surface.

Ghabry · 2017-09-09T11:36:24Z

src/game_actor.cpp

+	 *
+	 * Data::items (but not skills of items!)
+	 *
+	 */


have to update this comment

carstene1ns · 2017-09-09T18:16:16Z

src/game_actor.cpp

-	int final_level = Data::actors[actor_id - 1].final_level;
-	exp_list.resize(final_level, 0);;
-	for (int i = 1; i < final_level; ++i) {
+	exp_list.resize(99);


Magic number, maybe use GetMaxLevel().

Yes, this looks safe because GetLevel() is sanitized.

carstene1ns · 2017-09-09T18:16:30Z

src/game_actor.cpp

 }

 int Game_Actor::SetEquipment(int equip_type, int new_item_id) {
 	if (equip_type <= 0 || equip_type > (int) GetData().equipped.size())
 		return -1;

+


stray newline

carstene1ns · 2017-09-09T18:20:37Z

src/game_actor.cpp

@@ -583,7 +607,7 @@ int Game_Actor::GetLevel() const {
 }

 int Game_Actor::GetMaxLevel() const {
-	return Data::actors[actor_id - 1].final_level;
+	return std::max(1, std::min(GetActor().final_level, Player::IsRPG2k() ? 50 : 99));


Magic numbers, maybe we should add max_actor_level_* constants.

carstene1ns · 2017-09-09T18:25:51Z

src/game_actor.cpp

+
+		float bottom = top;
+		if (terrain) {
+			bottom = top + terrain->grid_b;


/ 13 is missing from old code.

carstene1ns · 2017-09-09T18:59:16Z

src/game_battler.cpp

+		const RPG::State& state = *ReaderUtil::GetElement(Data::states, i);
+
+		if (state.affect_agility) {
+			n = AffectParameter(state.affect_agility, base_agi);


This was affect_type before, wrong?

Ghabry · 2017-09-20T00:11:23Z

Fixed the reported problems and found more problems in the code.
Did a new commit to make the review easier.

carstene1ns · 2017-09-20T18:24:56Z

src/window_battlestatus.cpp

@@ -102,6 +103,7 @@ void Window_BattleStatus::RefreshGauge() {
 		}

 		for (int i = 0; i < item_max; ++i) {
+			// The always only contains valid battlers


carstene1ns · 2017-09-20T18:27:11Z

Looks good to me now apart from a few language issues in the comments, but this does not really matter.
e.g.
[…] array is resized dynamically

Not going to merge this, until at least a second review is available 😁.

Ghabry · 2017-09-20T18:28:23Z

And that review should take at least 30 minutes ;)

fdelapena · 2017-09-20T19:11:12Z

src/game_actor.cpp

@@ -18,6 +18,7 @@
 // Headers
 #include <algorithm>
 #include <sstream>
+#include <reader_util.h>


Other places use quotes for liblcf includes. (2)

fdelapena · 2017-09-20T19:22:32Z

src/game_battle.cpp

@@ -18,6 +18,7 @@
 #include <algorithm>
 #include <cassert>
 #include <deque>
+#include <reader_util.h>


Other places use quotes for liblcf includes. (3)

fdelapena · 2017-09-20T19:22:47Z

src/background.cpp

@@ -25,6 +25,8 @@
 #include "background.h"
 #include "bitmap.h"
 #include "main_data.h"
+#include "reader_util.h"


Other places use quotes for liblcf includes. (1)

Fixed all your notes except this one because it already uses quotes

fdelapena · 2017-09-20T19:23:36Z

src/game_battlealgorithm.cpp

@@ -20,6 +20,7 @@
 #include <cstdlib>
 #include <algorithm>
 #include <sstream>
+#include <reader_util.h>


Other places use quotes for liblcf includes. (4)

fdelapena · 2017-09-20T19:23:45Z

src/game_commonevent.cpp

@@ -16,6 +16,7 @@
 */

 // Headers
+#include <reader_util.h>


Other places use quotes for liblcf includes. (5)

fdelapena · 2017-09-20T19:27:22Z

src/window_shopparty.cpp

@@ -16,11 +16,13 @@
 */

 // Headers
+#include <reader_util.h>


Other places use quotes for liblcf includes. (27)

fdelapena · 2017-09-20T19:27:30Z

src/window_shopsell.cpp

@@ -18,12 +18,18 @@
 // Headers
 #include <iomanip>
 #include <sstream>
+#include <reader_util.h>


Other places use quotes for liblcf includes. (28)

fdelapena · 2017-09-20T19:27:41Z

src/window_skill.cpp

@@ -18,6 +18,7 @@
 // Headers
 #include <iomanip>
 #include <sstream>
+#include <reader_util.h>


Other places use quotes for liblcf includes. (29)

fdelapena · 2017-09-20T19:27:58Z

src/window_targetstatus.cpp

@@ -17,6 +17,7 @@

 // Headers
 #include <sstream>
+#include <reader_util.h>


Other places use quotes for liblcf includes. (30)

fdelapena · 2017-09-20T19:37:28Z

src/game_battler.cpp

@@ -19,6 +19,7 @@
 #include <algorithm>
 #include <cassert>
 #include <cmath>
+#include <reader_util.h>


Other places use quotes for liblcf includes. (31)

Ghabry · 2017-11-26T20:41:43Z

Thx for the feedback. Do you mean braces like here:
https://github.com/Ghabry/easyrpg-player/blob/5fdc7d78e07f6c8196a6f3405cfbda25c7f3c7e9/src/game_interpreter.cpp#L2823-L2830

Unfortunately the braces are required here because you can't declare variables in a case without introducing a scope :/

carstene1ns · 2017-11-26T21:06:26Z

Okay, did not know they were needed then.

carstene1ns · 2017-11-26T23:46:46Z

There seems to be a problem when returning to the gamebrowser from some games (maybe data is not cleared or cleared in the wrong order?): Warning: Removing invalid item # from party

Ghabry · 2017-11-27T12:33:04Z

Can you telll me a game where thus can be reproduced?

carstene1ns · 2017-11-27T14:45:09Z

Example games: Embric, Jojo, Lavender, Mondschein
To reproduce load a save or begin the game, press F12 and choose Exit in the Menu.

Ghabry · 2017-11-29T21:52:23Z

Had to add special handling for level 0 everywhere to prevent nullptr-dereferences due to new sanity checks (guess before it read out of bounds and somehow survived).

The last commit was a random find while testing Ara Fell.

…and exploit surface. This uses a new helper function ReaderUtil::GetElement for 1-based arrays in liblcf.

… check

thx carstene1ns

…ich is used by COLORS: Lost Memories. Replace GetData().level calls with GetLevel(). Fix EasyRPG#1248

…d or when returning to the Game Browser because Game_Screen accesses invalid picture data.

… (the state ID is the array index and the array value are the turns since the state was inflicted) the 1st check is sufficient.

…r window regains focus

Ghabry · 2017-12-06T21:05:05Z

Sorry for the rebase, was to prevent a future conflict. Had to remove a sanity check in "SetupBattle" because the original code has a bug and can be completely deleted. Will open an issue for this. #1314

The last unreviewed commit was:
"Add exception in the sanity check and special handling for Level 0"

…nd the idle and the action animation are the same.

carstene1ns · 2017-12-30T00:41:50Z

Well, have further investigated the reason why the message pops up:
When returning from game to title scene via F12, Data is not cleared when Player::ResetGameObjects() is called. Thus, the items are still valid. However, when returning to the gamebrowser, they are still there, but Data::Clear() has been called, so they are invalid.

As a fix, i think clearing the party items is necessary when leaving the game, either in ResetGameObjects() or where the party members are removed.

…tarted (and generated sanity check warnings in the game browser) Fix EasyRPG#1298 again

Ghabry · 2018-01-02T17:10:15Z

Looking forward to the bug reports. ;)

Ghabry commented Sep 9, 2017

View reviewed changes

src/game_actor.cpp Outdated

*

* Data::items (but not skills of items!)

*

*/

Copy link

Member Author

Ghabry Sep 9, 2017

Choose a reason for hiding this comment

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

have to update this comment

carstene1ns requested changes Sep 9, 2017

View reviewed changes

carstene1ns added this to the 0.5.3 milestone Sep 9, 2017

Ghabry force-pushed the sanitycheck branch from 041b5fb to f7ee1ef Compare September 20, 2017 00:10

carstene1ns reviewed Sep 20, 2017

View reviewed changes

carstene1ns approved these changes Sep 20, 2017

View reviewed changes

carstene1ns requested a review from fdelapena September 20, 2017 18:27

carstene1ns added the Refactor label Sep 20, 2017

fdelapena approved these changes Sep 20, 2017

View reviewed changes

Ghabry force-pushed the sanitycheck branch from f7ee1ef to e8f04e7 Compare September 23, 2017 22:25

Ghabry modified the milestones: 0.5.3, 0.5.4 Oct 1, 2017

Ghabry force-pushed the sanitycheck branch 3 times, most recently from e8f04e7 to e0bf97f Compare November 4, 2017 22:08

EasyRPG deleted a comment from JenkinsRPG Nov 4, 2017

carstene1ns approved these changes Nov 26, 2017

View reviewed changes

Ghabry added 13 commits December 6, 2017 22:01

Add sanity checks to all LCF data structure access to reduce crashes …

6a98d15

…and exploit surface. This uses a new helper function ReaderUtil::GetElement for 1-based arrays in liblcf.

Reword some interpreter warnings. Handle callstack limit.

67bbe04

Fix sanity check mistakes. Add additional checks and comments.

fc41741

Change reader_util include and improve warnings

bbac6fa

Validate actors returned from GetActor.

4dee0a6

Adopt sanity checks to latest BattleCommand changes

01d4515

Use zero-based index for Game_Actors to be consistent with the sanity…

cf5379f

… check

Fix style and code problems in sanity check.

d7869be

thx carstene1ns

Add exception in the sanity check and special handling for Level 0 wh…

d51ce72

…ich is used by COLORS: Lost Memories. Replace GetData().level calls with GetLevel(). Fix EasyRPG#1248

Fix a crash caused by the RPG2k3 1.12 picture code when F12 is presse…

8cd480c

…d or when returning to the Game Browser because Game_Screen accesses invalid picture data.

Remove 2nd state sanity check. Due to how the state array is designed…

b10fc91

… (the state ID is the array index and the array value are the turns since the state was inflicted) the 1st check is sufficient.

Fix incorrect index usage in Window ShopBuy

bb06cb4

Fix crash that happens when the last BGM failed to load and the Playe…

7be4595

…r window regains focus

Ghabry force-pushed the sanitycheck branch from a3ca50c to 7be4595 Compare December 6, 2017 21:02

fdelapena approved these changes Dec 6, 2017

View reviewed changes

Battle system: Fix out-of-bounds read when a battle charset is used a…

63c5112

…nd the idle and the action animation are the same.

Fix regression that prevented deletion of the items when a new game s…

69e2bf7

…tarted (and generated sanity check warnings in the game browser) Fix EasyRPG#1298 again

carstene1ns merged commit 34a6b9d into EasyRPG:master Jan 2, 2018

Ghabry deleted the sanitycheck branch May 7, 2018 14:06

Albeleon mentioned this pull request Jul 7, 2018

Battle System: Remaining, not implemented stuff #821

Closed

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sanity checks to all LCF data structure access #1259

Add sanity checks to all LCF data structure access #1259

Ghabry commented Sep 9, 2017

Ghabry Sep 9, 2017

carstene1ns Sep 9, 2017

Ghabry Sep 19, 2017

carstene1ns Sep 9, 2017

carstene1ns Sep 9, 2017

carstene1ns Sep 9, 2017

carstene1ns Sep 9, 2017

Ghabry commented Sep 20, 2017

carstene1ns Sep 20, 2017

carstene1ns commented Sep 20, 2017 •

edited

Loading

Ghabry commented Sep 20, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

Ghabry Sep 23, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

fdelapena Sep 20, 2017

Ghabry commented Nov 26, 2017

carstene1ns commented Nov 26, 2017

carstene1ns commented Nov 26, 2017

Ghabry commented Nov 27, 2017

carstene1ns commented Nov 27, 2017

Ghabry commented Nov 29, 2017

Ghabry commented Dec 6, 2017 •

edited

Loading

carstene1ns commented Dec 30, 2017

Ghabry commented Jan 2, 2018

Add sanity checks to all LCF data structure access #1259

Add sanity checks to all LCF data structure access #1259

Conversation

Ghabry commented Sep 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ghabry commented Sep 20, 2017

Choose a reason for hiding this comment

carstene1ns commented Sep 20, 2017 • edited Loading

Ghabry commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ghabry commented Nov 26, 2017

carstene1ns commented Nov 26, 2017

carstene1ns commented Nov 26, 2017

Ghabry commented Nov 27, 2017

carstene1ns commented Nov 27, 2017

Ghabry commented Nov 29, 2017

Ghabry commented Dec 6, 2017 • edited Loading

carstene1ns commented Dec 30, 2017

Ghabry commented Jan 2, 2018

carstene1ns commented Sep 20, 2017 •

edited

Loading

Ghabry commented Dec 6, 2017 •

edited

Loading