From 6ad4b220d75a32ea8df959e9a7b7f1ecb21d0c26 Mon Sep 17 00:00:00 2001 From: Ivan Mogilko Date: Thu, 24 Aug 2023 23:34:24 +0300 Subject: [PATCH] Engine: add object and char assertions in Get/SetProperty functions Since older commit 5f3b8eb509c9535c75b401f9b7257f7c7acd0e6d the properties are stored in vectors, strictly resized to the number of characters and objects. But the script functions that access these do not do any assertion whether the object or char is valid, which could cause bad mem access in case script loops over a array of objects of MAX_OBJECTS size, or tries to access something using Object* pointer from another room. --- Engine/ac/character.cpp | 41 ++++++++++++++++++++++++++++--------- Engine/ac/character.h | 6 +++++- Engine/ac/global_object.cpp | 2 ++ Engine/ac/object.cpp | 24 +++++++++++++++++----- Engine/ac/object.h | 5 ++++- 5 files changed, 61 insertions(+), 17 deletions(-) diff --git a/Engine/ac/character.cpp b/Engine/ac/character.cpp index 56e6c85005e..6f0e26ddf66 100644 --- a/Engine/ac/character.cpp +++ b/Engine/ac/character.cpp @@ -103,6 +103,19 @@ int numLipLines = 0, curLipLine = -1, curLipLinePhoneme = 0; // **** CHARACTER: FUNCTIONS **** +bool is_valid_character(int char_id) +{ + return ((char_id >= 0) && (char_id < game.numcharacters)); +} + +bool AssertCharacter(const char *apiname, int char_id) +{ + if ((char_id >= 0) && (char_id < game.numcharacters)) + return true; + debug_script_warn("%s: invalid character id %d (range is 0..%d)", apiname, char_id, game.numcharacters - 1); + return false; +} + void Character_AddInventory(CharacterInfo *chaa, ScriptInvItem *invi, int addIndex) { int ee; @@ -1050,25 +1063,38 @@ void Character_RunInteraction(CharacterInfo *chaa, int mood) { // **** CHARACTER: PROPERTIES **** -int Character_GetProperty(CharacterInfo *chaa, const char *property) { - +int Character_GetProperty(CharacterInfo *chaa, const char *property) +{ + if (!AssertCharacter("Character.GetProperty", chaa->index_id)) + return 0; return get_int_property(game.charProps[chaa->index_id], play.charProps[chaa->index_id], property); - } -void Character_GetPropertyText(CharacterInfo *chaa, const char *property, char *bufer) { + +void Character_GetPropertyText(CharacterInfo *chaa, const char *property, char *bufer) +{ + if (!AssertCharacter("Character.GetPropertyText", chaa->index_id)) + return; get_text_property(game.charProps[chaa->index_id], play.charProps[chaa->index_id], property, bufer); } -const char* Character_GetTextProperty(CharacterInfo *chaa, const char *property) { + +const char* Character_GetTextProperty(CharacterInfo *chaa, const char *property) +{ + if (!AssertCharacter("Character.GetTextProperty", chaa->index_id)) + return nullptr; return get_text_property_dynamic_string(game.charProps[chaa->index_id], play.charProps[chaa->index_id], property); } bool Character_SetProperty(CharacterInfo *chaa, const char *property, int value) { + if (!AssertCharacter("Character.SetProperty", chaa->index_id)) + return false; return set_int_property(play.charProps[chaa->index_id], property, value); } bool Character_SetTextProperty(CharacterInfo *chaa, const char *property, const char *value) { + if (!AssertCharacter("Character.SetTextProperty", chaa->index_id)) + return false; return set_text_property(play.charProps[chaa->index_id], property, value); } @@ -2028,11 +2054,6 @@ void walk_or_move_character(CharacterInfo *chaa, int x, int y, int blocking, int } -int is_valid_character(int newchar) { - if ((newchar < 0) || (newchar >= game.numcharacters)) return 0; - return 1; -} - int wantMoveNow (CharacterInfo *chi, CharacterExtras *chex) { // check most likely case first if ((chex->zoom == 100) || ((chi->flags & CHF_SCALEMOVESPEED) == 0)) diff --git a/Engine/ac/character.h b/Engine/ac/character.h index 211f3c13cc0..050d1e05b4a 100644 --- a/Engine/ac/character.h +++ b/Engine/ac/character.h @@ -28,6 +28,11 @@ // **** CHARACTER: FUNCTIONS **** +bool is_valid_character(int char_id); +// Asserts the character ID is valid, +// if not then prints a warning to the log; returns assertion result +bool AssertCharacter(const char *apiname, int char_id); + void Character_AddInventory(CharacterInfo *chaa, ScriptInvItem *invi, int addIndex); void Character_AddWaypoint(CharacterInfo *chaa, int x, int y); void Character_Animate(CharacterInfo *chaa, int loop, int delay, int repeat, @@ -186,7 +191,6 @@ void find_nearest_walkable_area (int *xx, int *yy); void walk_character(int chac,int tox,int toy,int ignwal, bool autoWalkAnims); void FindReasonableLoopForCharacter(CharacterInfo *chap); void walk_or_move_character(CharacterInfo *chaa, int x, int y, int blocking, int direct, bool isWalk); -int is_valid_character(int newchar); int wantMoveNow (CharacterInfo *chi, CharacterExtras *chex); void setup_player_character(int charid); Common::Bitmap *GetCharacterImage(int charid, bool *is_original = nullptr); diff --git a/Engine/ac/global_object.cpp b/Engine/ac/global_object.cpp index 8fdf003a6cb..ae3e5c8ec1a 100644 --- a/Engine/ac/global_object.cpp +++ b/Engine/ac/global_object.cpp @@ -545,6 +545,8 @@ int GetObjectProperty (int hss, const char *property) void GetObjectPropertyText (int item, const char *property, char *bufer) { + if (!AssertObject("GetObjectPropertyText", item)) + return; get_text_property(thisroom.Objects[item].Properties, croom->objProps[item], property, bufer); } diff --git a/Engine/ac/object.cpp b/Engine/ac/object.cpp index ce1b82cbcb7..60956e4d442 100644 --- a/Engine/ac/object.cpp +++ b/Engine/ac/object.cpp @@ -54,6 +54,19 @@ extern IGraphicsDriver *gfxDriver; extern CCObject ccDynamicObject; +bool is_valid_object(int obj_id) +{ + return (obj_id >= 0) && (static_cast(obj_id) < croom->numobj); +} + +bool AssertObject(const char *apiname, int obj_id) +{ + if ((obj_id >= 0) && (static_cast(obj_id) < croom->numobj)) + return true; + debug_script_warn("%s: invalid object id %d (range is 0..%d)", apiname, obj_id, croom->numobj - 1); + return false; +} + int Object_IsCollidingWithObject(ScriptObject *objj, ScriptObject *obj2) { return AreObjectsColliding(objj->id, obj2->id); } @@ -73,11 +86,6 @@ ScriptObject *GetObjectAtRoom(int x, int y) return &scrObj[hsnum]; } -int is_valid_object(int obtest) { - if ((obtest < 0) || (static_cast(obtest) >= croom->numobj)) return 0; - return 1; -} - void Object_Tint(ScriptObject *objj, int red, int green, int blue, int saturation, int luminance) { SetObjectTint(objj->id, red, green, blue, saturation, luminance); } @@ -459,16 +467,22 @@ void Object_GetPropertyText(ScriptObject *objj, const char *property, char *bufe const char* Object_GetTextProperty(ScriptObject *objj, const char *property) { + if (!AssertObject("Object.GetTextProperty", objj->id)) + return nullptr; return get_text_property_dynamic_string(thisroom.Objects[objj->id].Properties, croom->objProps[objj->id], property); } bool Object_SetProperty(ScriptObject *objj, const char *property, int value) { + if (!AssertObject("Object.SetProperty", objj->id)) + return false; return set_int_property(croom->objProps[objj->id], property, value); } bool Object_SetTextProperty(ScriptObject *objj, const char *property, const char *value) { + if (!AssertObject("Object.SetTextProperty", objj->id)) + return false; return set_text_property(croom->objProps[objj->id], property, value); } diff --git a/Engine/ac/object.h b/Engine/ac/object.h index 95a5b3a671d..bde2347dac9 100644 --- a/Engine/ac/object.h +++ b/Engine/ac/object.h @@ -26,7 +26,10 @@ namespace AGS { namespace Common { class Bitmap; } } using namespace AGS; // FIXME later -int is_valid_object(int obtest); +bool is_valid_object(int obj_id); +// Asserts the object ID is valid in the current room, +// if not then prints a warning to the log; returns assertion result +bool AssertObject(const char *apiname, int obj_id); int Object_IsCollidingWithObject(ScriptObject *objj, ScriptObject *obj2); ScriptObject *GetObjectAtScreen(int xx, int yy); void Object_Tint(ScriptObject *objj, int red, int green, int blue, int saturation, int luminance);