Engine: add object and char assertions in Get/SetProperty functions

Since older commit 5f3b8eb 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.

Author: ivan-mogilko

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))

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);

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);
 }
 

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<uint32_t>(obj_id) < croom->numobj);
+}
+
+bool AssertObject(const char *apiname, int obj_id)
+{
+    if ((obj_id >= 0) && (static_cast<uint32_t>(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<uint32_t>(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);
 }
 

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);