Merge pull request #2945 from cppcooper/channel-safely

Fix out-of-bounds error in Channel safely

docs/changelog.txt

@@ -58,6 +58,7 @@ changelog.txt uses a syntax similar to RST, with a few special sequences:
 - ``Buildings::StockpileIterator``: fix check for stockpile items on block boundary.
 - `tailor`: block making clothing sized for toads; make replacement clothing orders use the size of the wearer, not the size of the garment
 -@ `confirm`: fix fps drop when enabled
+- `channel-safely`: fix an out of bounds error regarding the REPORT event listener receiving (presumably) stale id's
 
 ## Misc Improvements
 - `autobutcher`: logs activity to the console terminal instead of making disruptive in-game announcements

plugins/channel-safely/channel-groups.cpp

@@ -21,24 +21,27 @@ void ChannelJobs::load_channel_jobs() {
 }
 
 bool ChannelJobs::has_cavein_conditions(const df::coord &map_pos) {
-    auto p = map_pos;
+    if likely(Maps::isValidTilePos(map_pos)) {
-    auto ttype = *Maps::getTileType(p);
+        auto p = map_pos;
-    if (!DFHack::isOpenTerrain(ttype)) {
+        auto ttype = *Maps::getTileType(p);
-        // check shared neighbour for cave-in conditions
+        if (!DFHack::isOpenTerrain(ttype)) {
-        df::coord neighbours[4];
+            // check shared neighbour for cave-in conditions
-        get_connected_neighbours(map_pos, neighbours);
+            df::coord neighbours[4];
-        int connectedness = 4;
+            get_connected_neighbours(map_pos, neighbours);
-        for (auto n: neighbours) {
+            int connectedness = 4;
-            if (active.count(n) || DFHack::isOpenTerrain(*Maps::getTileType(n))) {
+            for (auto n: neighbours) {
-                connectedness--;
+                if (!Maps::isValidTilePos(n) || active.count(n) || DFHack::isOpenTerrain(*Maps::getTileType(n))) {
+                    connectedness--;
+                }
             }
-        }
+            if (!connectedness) {
-        if (!connectedness) {
+                // do what?
-            // do what?
+                p.z--;
-            p.z--;
+                if (!Maps::isValidTilePos(p)) return false;
-            ttype = *Maps::getTileType(p);
+                ttype = *Maps::getTileType(p);
-            if (DFHack::isOpenTerrain(ttype) || DFHack::isFloorTerrain(ttype)) {
+                if (DFHack::isOpenTerrain(ttype) || DFHack::isFloorTerrain(ttype)) {
-                return true;
+                    return true;
+                }
             }
         }
     }
@@ -88,6 +91,7 @@ void ChannelGroups::add(const df::coord &map_pos) {
     DEBUG(groups).print("    add(" COORD ")\n", COORDARGS(map_pos));
     // and so we begin iterating the neighbours
     for (auto &neighbour: neighbors) {
+        if unlikely(!Maps::isValidTilePos(neighbour)) continue;
         // go to the next neighbour if this one doesn't have a group
         if (!groups_map.count(neighbour)) {
             TRACE(groups).print(" -> neighbour is not designated\n");

plugins/channel-safely/channel-safely-plugin.cpp

@@ -112,6 +112,10 @@ enum SettingConfigData {
 
 // dig-now.cpp
 df::coord simulate_fall(const df::coord &pos) {
+    if unlikely(!Maps::isValidTilePos(pos)) {
+        ERR(plugin).print("Error: simulate_fall(" COORD ") - invalid coordinate\n", COORDARGS(pos));
+        return {};
+    }
     df::coord resting_pos(pos);
 
     while (Maps::ensureTileBlock(resting_pos)) {
@@ -130,6 +134,7 @@ df::coord simulate_area_fall(const df::coord &pos) {
     get_neighbours(pos, neighbours);
     df::coord lowest = simulate_fall(pos);
     for (auto p : neighbours) {
+        if unlikely(!Maps::isValidTilePos(p)) continue;
         auto nlow = simulate_fall(p);
         if (nlow.z < lowest.z) {
             lowest = nlow;
@@ -299,10 +304,11 @@ namespace CSP {
         int32_t tick = df::global::world->frame_counter;
         auto report_id = (int32_t)(intptr_t(r));
         if (df::global::world) {
-            std::vector<df::report*> &reports = df::global::world->status.reports;
+            df::report* report = df::report::find(report_id);
-            size_t idx = -1;
+            if (!report) {
-            idx = df::report::binsearch_index(reports, report_id);
+                WARN(plugin).print("Error: NewReportEvent() received an invalid report_id - a report* cannot be found\n");
-            df::report* report = reports.at(idx);
+                return;
+            }
             switch (report->type) {
                 case announcement_type::CANCEL_JOB:
                     if (config.insta_dig) {

plugins/channel-safely/include/inlines.h

@@ -64,11 +64,13 @@ inline uint8_t count_accessibility(const df::coord &unit_pos, const df::coord &m
     get_connected_neighbours(map_pos, connections);
     uint8_t accessibility = Maps::canWalkBetween(unit_pos, map_pos) ? 1 : 0;
     for (auto n: neighbours) {
+        if unlikely(!Maps::isValidTilePos(n)) continue;
         if (Maps::canWalkBetween(unit_pos, n)) {
             accessibility++;
         }
     }
     for (auto n : connections) {
+        if unlikely(Maps::isValidTilePos(n)) continue;
         if (Maps::canWalkBetween(unit_pos, n)) {
             accessibility++;
         }
@@ -77,22 +79,22 @@ inline uint8_t count_accessibility(const df::coord &unit_pos, const df::coord &m
 }
 
 inline bool isEntombed(const df::coord &unit_pos, const df::coord &map_pos) {
-    if (Maps::canWalkBetween(unit_pos, map_pos)) {
+    if likely(Maps::canWalkBetween(unit_pos, map_pos)) {
         return false;
     }
     df::coord neighbours[8];
     get_neighbours(map_pos, neighbours);
     return std::all_of(neighbours+0, neighbours+8, [&unit_pos](df::coord n) {
-        return !Maps::canWalkBetween(unit_pos, n);
+        return !Maps::isValidTilePos(n) || !Maps::canWalkBetween(unit_pos, n);
     });
 }
 
 inline bool is_dig_job(const df::job* job) {
-    return job->job_type == df::job_type::Dig || job->job_type == df::job_type::DigChannel;
+    return job && (job->job_type == df::job_type::Dig || job->job_type == df::job_type::DigChannel);
 }
 
 inline bool is_channel_job(const df::job* job) {
-    return job->job_type == df::job_type::DigChannel;
+    return job && (job->job_type == df::job_type::DigChannel);
 }
 
 inline bool is_group_job(const ChannelGroups &groups, const df::job* job) {
@@ -111,34 +113,48 @@ inline bool is_safe_fall(const df::coord &map_pos) {
     df::coord below(map_pos);
     for (uint8_t zi = 0; zi < config.fall_threshold; ++zi) {
         below.z--;
+        // falling out of bounds is probably considerably unsafe for a dwarf
+        if unlikely(!Maps::isValidTilePos(below)) {
+            return false;
+        }
+        // if we require vision, and we can't see below.. we'll need to assume it's safe to get anything done
         if (config.require_vision && Maps::getTileDesignation(below)->bits.hidden) {
-            return true; //we require vision, and we can't see below.. so we gotta assume it's safe
+            return true;
         }
+        // finally, if we're not looking at open space (air to fall through) it's safe to fall to
         df::tiletype type = *Maps::getTileType(below);
         if (!DFHack::isOpenTerrain(type)) {
             return true;
         }
     }
+    // we exceeded the fall threshold, so it's not a safe fall
     return false;
 }
 
 inline bool is_safe_to_dig_down(const df::coord &map_pos) {
     df::coord pos(map_pos);
 
+    // todo: probably should rely on is_safe_fall, it looks like it could be simplified a great deal
     for (uint8_t zi = 0; zi <= config.fall_threshold; ++zi) {
-        // assume safe if we can't see and need vision
+        // if we're digging out of bounds, the game can handle that (hopefully)
+        if unlikely(!Maps::isValidTilePos(pos)) {
+            return true;
+        }
+        // if we require vision, and we can't see the tiles in question.. we'll need to assume it's safe to dig to get anything done
         if (config.require_vision && Maps::getTileDesignation(pos)->bits.hidden) {
             return true;
         }
+
         df::tiletype type = *Maps::getTileType(pos);
         if (zi == 0 && DFHack::isOpenTerrain(type)) {
+            // todo: remove? this is probably not useful.. and seems like the only considerable difference to is_safe_fall (aside from where each stops looking)
             // the starting tile is open space, that's obviously not safe
             return false;
         } else if (!DFHack::isOpenTerrain(type)) {
             // a tile after the first one is not open space
             return true;
         }
-        pos.z--;
+        pos.z--; // todo: this can probably move to the beginning of the loop
     }
     return false;
 }