From 76d6a3536b756926d15c4b50f4b496a049caa10f Mon Sep 17 00:00:00 2001 From: Misa Date: Wed, 9 Sep 2020 22:31:09 -0700 Subject: [PATCH] Bounds check all entity getters that can return 0 The entity getters I'm referring to are entityclass::getscm(), entityclass::getlineat(), entityclass::getcrewman(), and entityclass::getcustomcrewman(). Even though the player should always exist, and the player should always be indice 0, I wouldn't want to make that assumption. I've been wrong before. Also, these functions returning 0 lull you into a false sense of security. If you assume that commands using these functions are fine, you'll forget about the fact that `i` in those commands could be potentially anything, given an invalid argument. In fact, it's possible to index createactivityzone(), flipgravity(), and customposition() out-of-bounds by setting `i` to anything! Well, WAS possible. I fixed it so now they can't. Furthermore, in the game.scmmoveme block in gamelogic(), obj.getplayer() wasn't even checked, even though it's been checked in all other places. I only caught it just now because I wanted to bounds-check all usages of obj.getscm(), too, and that game.scmmove block also used obj.getscm() without bounds-checking it as well. --- desktop_version/src/Entity.cpp | 155 ++++++++++++++++------------- desktop_version/src/Logic.cpp | 35 +++++-- desktop_version/src/Map.cpp | 7 +- desktop_version/src/Otherlevel.cpp | 9 +- desktop_version/src/Script.cpp | 26 +++-- 5 files changed, 136 insertions(+), 96 deletions(-) diff --git a/desktop_version/src/Entity.cpp b/desktop_version/src/Entity.cpp index b22e63be..94d679b7 100644 --- a/desktop_version/src/Entity.cpp +++ b/desktop_version/src/Entity.cpp @@ -2813,110 +2813,125 @@ bool entityclass::updateentities( int i ) { //11-15 means to follow a specific character, in crew order (cyan, purple, yellow, red, green, blue) int j=getcrewman(1); //purple - if (entities[j].xp > entities[i].xp + 5) + if (INBOUNDS_VEC(j, entities)) { - entities[i].dir = 1; - } - else if (entities[j].xp < entities[i].xp - 5) - { - entities[i].dir = 0; - } + if (entities[j].xp > entities[i].xp + 5) + { + entities[i].dir = 1; + } + else if (entities[j].xp < entities[i].xp - 5) + { + entities[i].dir = 0; + } - if (entities[j].xp > entities[i].xp + 45) - { - entities[i].ax = 3; - } - else if (entities[j].xp < entities[i].xp - 45) - { - entities[i].ax = -3; + if (entities[j].xp > entities[i].xp + 45) + { + entities[i].ax = 3; + } + else if (entities[j].xp < entities[i].xp - 45) + { + entities[i].ax = -3; + } } } else if (entities[i].state == 12) { //11-15 means to follow a specific character, in crew order (cyan, purple, yellow, red, green, blue) int j=getcrewman(2); //yellow - if (entities[j].xp > entities[i].xp + 5) + if (INBOUNDS_VEC(j, entities)) { - entities[i].dir = 1; - } - else if (entities[j].xp < entities[i].xp - 5) - { - entities[i].dir = 0; - } + if (entities[j].xp > entities[i].xp + 5) + { + entities[i].dir = 1; + } + else if (entities[j].xp < entities[i].xp - 5) + { + entities[i].dir = 0; + } - if (entities[j].xp > entities[i].xp + 45) - { - entities[i].ax = 3; - } - else if (entities[j].xp < entities[i].xp - 45) - { - entities[i].ax = -3; + if (entities[j].xp > entities[i].xp + 45) + { + entities[i].ax = 3; + } + else if (entities[j].xp < entities[i].xp - 45) + { + entities[i].ax = -3; + } } } else if (entities[i].state == 13) { //11-15 means to follow a specific character, in crew order (cyan, purple, yellow, red, green, blue) int j=getcrewman(3); //red - if (entities[j].xp > entities[i].xp + 5) + if (INBOUNDS_VEC(j, entities)) { - entities[i].dir = 1; - } - else if (entities[j].xp < entities[i].xp - 5) - { - entities[i].dir = 0; - } + if (entities[j].xp > entities[i].xp + 5) + { + entities[i].dir = 1; + } + else if (entities[j].xp < entities[i].xp - 5) + { + entities[i].dir = 0; + } - if (entities[j].xp > entities[i].xp + 45) - { - entities[i].ax = 3; - } - else if (entities[j].xp < entities[i].xp - 45) - { - entities[i].ax = -3; + if (entities[j].xp > entities[i].xp + 45) + { + entities[i].ax = 3; + } + else if (entities[j].xp < entities[i].xp - 45) + { + entities[i].ax = -3; + } } } else if (entities[i].state == 14) { //11-15 means to follow a specific character, in crew order (cyan, purple, yellow, red, green, blue) int j=getcrewman(4); //green - if (entities[j].xp > entities[i].xp + 5) + if (INBOUNDS_VEC(j, entities)) { - entities[i].dir = 1; - } - else if (entities[j].xp < entities[i].xp - 5) - { - entities[i].dir = 0; - } + if (entities[j].xp > entities[i].xp + 5) + { + entities[i].dir = 1; + } + else if (entities[j].xp < entities[i].xp - 5) + { + entities[i].dir = 0; + } - if (entities[j].xp > entities[i].xp + 45) - { - entities[i].ax = 3; - } - else if (entities[j].xp < entities[i].xp - 45) - { - entities[i].ax = -3; + if (entities[j].xp > entities[i].xp + 45) + { + entities[i].ax = 3; + } + else if (entities[j].xp < entities[i].xp - 45) + { + entities[i].ax = -3; + } } } else if (entities[i].state == 15) { //11-15 means to follow a specific character, in crew order (cyan, purple, yellow, red, green, blue) int j=getcrewman(5); //blue - if (entities[j].xp > entities[i].xp + 5) + if (INBOUNDS_VEC(j, entities)) { - entities[i].dir = 1; - } - else if (entities[j].xp < entities[i].xp - 5) - { - entities[i].dir = 0; - } + if (entities[j].xp > entities[i].xp + 5) + { + entities[i].dir = 1; + } + else if (entities[j].xp < entities[i].xp - 5) + { + entities[i].dir = 0; + } - if (entities[j].xp > entities[i].xp + 45) - { - entities[i].ax = 3; - } - else if (entities[j].xp < entities[i].xp - 45) - { - entities[i].ax = -3; + if (entities[j].xp > entities[i].xp + 45) + { + entities[i].ax = 3; + } + else if (entities[j].xp < entities[i].xp - 45) + { + entities[i].ax = -3; + } } } else if (entities[i].state == 16) diff --git a/desktop_version/src/Logic.cpp b/desktop_version/src/Logic.cpp index 9a29d8f0..629f4642 100644 --- a/desktop_version/src/Logic.cpp +++ b/desktop_version/src/Logic.cpp @@ -769,12 +769,16 @@ void gamelogic() } else if (game.swngame == 3) //extend line { - obj.entities[obj.getlineat(84 - 32)].w += 24; - if (obj.entities[obj.getlineat(84 - 32)].w > 332) + int line = obj.getlineat(84 - 32); + if (INBOUNDS_VEC(line, obj.entities)) { - obj.entities[obj.getlineat(84 - 32)].w = 332; - game.swngame = 2; - graphics.kludgeswnlinewidth = true; + obj.entities[line].w += 24; + if (obj.entities[line].w > 332) + { + obj.entities[line].w = 332; + game.swngame = 2; + graphics.kludgeswnlinewidth = true; + } } } else if (game.swngame == 4) //create top line @@ -786,12 +790,16 @@ void gamelogic() } else if (game.swngame == 5) //remove line { - obj.entities[obj.getlineat(148 + 32)].xp += 24; - if (obj.entities[obj.getlineat(148 + 32)].xp > 320) + int line = obj.getlineat(148 + 32); + if (INBOUNDS_VEC(line, obj.entities)) { - obj.removeentity(obj.getlineat(148 + 32)); - game.swnmode = false; - game.swngame = 6; + obj.entities[line].xp += 24; + if (obj.entities[line].xp > 320) + { + obj.removeentity(line); + game.swnmode = false; + game.swngame = 6; + } } } else if (game.swngame == 6) //Init the super gravitron @@ -1587,7 +1595,12 @@ void gamelogic() if (game.scmmoveme) { - obj.entities[obj.getscm()].xp = obj.entities[obj.getplayer()].xp; + int scm = obj.getscm(); + int player = obj.getplayer(); + if (INBOUNDS_VEC(scm, obj.entities) && INBOUNDS_VEC(player, obj.entities)) + { + obj.entities[scm].xp = obj.entities[player].xp; + } game.scmmoveme = false; } break; diff --git a/desktop_version/src/Map.cpp b/desktop_version/src/Map.cpp index 3309b0af..7d44e2cc 100644 --- a/desktop_version/src/Map.cpp +++ b/desktop_version/src/Map.cpp @@ -2077,8 +2077,11 @@ void mapclass::loadlevel(int rx, int ry) //A slight varation - she's upside down obj.createentity(249, 62, 18, 16, 0, 18); int j = obj.getcrewman(5); - obj.entities[j].rule = 7; - obj.entities[j].tile +=6; + if (INBOUNDS_VEC(j, obj.entities)) + { + obj.entities[j].rule = 7; + obj.entities[j].tile +=6; + } //What script do we use? obj.createblock(5, 249-32, 0, 32+32+32, 240, 5); } diff --git a/desktop_version/src/Otherlevel.cpp b/desktop_version/src/Otherlevel.cpp index 5ca36d07..69256f17 100644 --- a/desktop_version/src/Otherlevel.cpp +++ b/desktop_version/src/Otherlevel.cpp @@ -3,6 +3,7 @@ #include "Game.h" #include "Entity.h" #include "MakeAndPlay.h" +#include "UtilityClass.h" const short* otherlevelclass::loadlevel(int rx, int ry) { @@ -8892,8 +8893,12 @@ const short* otherlevelclass::loadlevel(int rx, int ry) //violet obj.createentity(83, 126, 18, 20, 0, 18); - obj.entities[obj.getcrewman(1)].rule = 7; - obj.entities[obj.getcrewman(1)].tile +=6; + int crewman = obj.getcrewman(1); + if (INBOUNDS_VEC(crewman, obj.entities)) + { + obj.entities[crewman].rule = 7; + obj.entities[crewman].tile +=6; + } obj.createblock(5, 83 - 32, 0, 32 + 32 + 32, 240, 1); } result = contents; diff --git a/desktop_version/src/Script.cpp b/desktop_version/src/Script.cpp index 82a6c77e..d1d6b3e5 100644 --- a/desktop_version/src/Script.cpp +++ b/desktop_version/src/Script.cpp @@ -582,7 +582,7 @@ void scriptclass::run() } //next is whether to position above or below - if (words[2] == "above") + if (INBOUNDS_VEC(i, obj.entities) && words[2] == "above") { if (j == 1) //left { @@ -595,7 +595,7 @@ void scriptclass::run() texty = obj.entities[i].yp - 18 - (txt.size() * 8); } } - else + else if (INBOUNDS_VEC(i, obj.entities)) { if (j == 1) //left { @@ -922,11 +922,11 @@ void scriptclass::run() obj.customcrewmoods[1]=ss_toi(words[2]); } - if (ss_toi(words[2]) == 0) + if (INBOUNDS_VEC(i, obj.entities) && ss_toi(words[2]) == 0) { obj.entities[i].tile = 0; } - else + else if (INBOUNDS_VEC(i, obj.entities)) { obj.entities[i].tile = 144; } @@ -1001,12 +1001,12 @@ void scriptclass::run() i=obj.getcrewman(1); } - if (obj.entities[i].rule == 7) + if (INBOUNDS_VEC(i, obj.entities) && obj.entities[i].rule == 7) { obj.entities[i].rule = 6; obj.entities[i].tile = 0; } - else if (obj.getplayer() != i) // Don't destroy player entity + else if (INBOUNDS_VEC(i, obj.entities) && obj.getplayer() != i) // Don't destroy player entity { obj.entities[i].rule = 7; obj.entities[i].tile = 6; @@ -1863,13 +1863,14 @@ void scriptclass::run() i=1; } - if (i == 4) + int crewman = obj.getcrewman(i); + if (INBOUNDS_VEC(crewman, obj.entities) && i == 4) { - obj.createblock(5, obj.entities[obj.getcrewman(i)].xp - 32, obj.entities[obj.getcrewman(i)].yp-20, 96, 60, i); + obj.createblock(5, obj.entities[crewman].xp - 32, obj.entities[crewman].yp-20, 96, 60, i); } - else + else if (INBOUNDS_VEC(crewman, obj.entities)) { - obj.createblock(5, obj.entities[obj.getcrewman(i)].xp - 32, 0, 96, 240, i); + obj.createblock(5, obj.entities[crewman].xp - 32, 0, 96, 240, i); } } else if (words[0] == "createrescuedcrew") @@ -2097,7 +2098,10 @@ void scriptclass::run() obj.createentity(200, 153, 18, r, 0, 19, 30); i = obj.getcrewman(game.lastsaved); - obj.entities[i].dir = 1; + if (INBOUNDS_VEC(i, obj.entities)) + { + obj.entities[i].dir = 1; + } } else if (words[0] == "specialline") {