fix three plugin crash/UB bugs; test coverage for 14 plugins

docs/changelog.txt

@@ -66,25 +66,30 @@ Template for new versions:
 
 ## Fixes
 - Fix broken weather lookup in ``World::ReadCurrentWeather``
+- Fix ``dfhack.items.createItem`` making glob items of an incorrect size
 - Fixed a possible hang or assertion failure when pressing a hotkey while a DFHack GUI window was open but unfocused
+- Fixed persistent site data API (``dfhack.persistent.saveSiteData``/``getSiteData``) failing on newly reclaimed fortresses until the first save
 - In ``Screen`` module, Fix out-of-bounds color table access when Lua pens use ``COLOR_RESET``
 - rename the ``stockpiles`` tool tag to ``stockpile`` so it no longer collides with the `stockpiles` plugin in `gui/launcher`
 - ``Units::teleport``: update unit occupancy on the full 3x3 footprint of EQUIPMENT units (e.g. wagons) instead of only their center tile
 - ``Units::teleport``: only clear tile unit occupancy flags when no other unit of the same kind remains on the tile
+- `3dveins`: fix crash when a tile's biome cannot be resolved
 - `3dveins`: fix failure on embarks that cross midmap tiles
 - `aquifer`: make ``--skip-top`` and top-relative ``--levels`` take effect for the ``drain``, ``convert``, and ``add`` actions instead of being silently ignored
 - `autodump`: ``destroy`` no longer leaves the contents of destroyed containers in limbo or crashes when destroying unit-held items
 - `buildingplan`: fix roller material estimate asking for one chain per tile instead of one chain total
 - `buildingplan`: fix "Unlink all" only unlinking some mechanisms (or crashing) when freeing mechanisms from the building
+- `export-world-map`: fix crash exporting site data for a site whose owner has an undefined race
 - `overlay`: overlay hotkeys no longer steal typed characters while the game is capturing text input, e.g. when renaming a building
 - `sort`: fix the Places/Workshops and task searches missing workshops by their current display names (e.g. "Stoneworker's Workshop", "Screw Press")
 - `stocks`: overlay now resets scroll position when collapsing categories so the item list is no longer left blank and unscrollable
 - `strangemood`: updated to match behavior of current DF version
 - `tiletypes`: remove plants (including trees) rooted on a tile that is painted into something that cannot host them, so cleared ground no longer regrows floating trees
 - `gui/quickfort`: show smoothing designations immediately after applying a blueprint
 - `timestream`: deal properly with units who have breathing difficulties
-- Fixed persistent site data API (``dfhack.persistent.saveSiteData``/``getSiteData``) failing on newly reclaimed fortresses until the first save
-- Fix ``dfhack.items.createItem`` making glob items of an incorrect size
+- `stocks`: overlay now resets scroll position when collapsing categories so the item list is no longer left blank and unscrollable
+- `strangemood`: fix crash when ``--id`` is given a non-numeric unit id
+- `suspendmanager`: fix out-of-bounds parameter read when ``set`` is given too few arguments
 
 ## Misc Improvements
 - Added ``Coord2d`` and ``Coord3d`` C++ templates, providing a standard set operations for 2-tuples and 3-tuples of any numeric type

plugins/3dveins.cpp

@@ -655,7 +655,9 @@ bool GeoBiome::init_layers()
 GeoLayer *VeinGenerator::mapLayer(Block *pb, df::coord2d tile)
 {
     int idx = pb->biomeIndexAt(tile);
-    GeoBiome *biome = biome_by_idx.at(idx);
+    if (idx < 0)
+        return NULL;
+    GeoBiome *biome = biome_by_idx[idx];
 
     int lidx = pb->layerIndexAt(tile);
     if (unsigned(lidx) >= biome->layers.size())

plugins/export-world-map.cpp

@@ -218,9 +218,10 @@ static command_result export_sites(color_ostream &out)
         df::creature_raw *race = nullptr;
         if (owner){
             race = df::creature_raw::find(owner->race);
-            DEBUG(warning, out).print("owner ({}) of site ({}) has undefined race ({})", owner->id, site->id, owner->race);
             if (!race)            {
-                df::creature_raw::find(civ->race);
+                DEBUG(warning, out).print("owner ({}) of site ({}) has undefined race ({})\n", owner->id, site->id, owner->race);
+                if (civ)
+                    race = df::creature_raw::find(civ->race);
             }
         }
 

plugins/strangemood.cpp

@@ -4,6 +4,7 @@
 #include "Console.h"
 #include "DataDefs.h"
 #include "Export.h"
+#include "MiscUtils.h"
 #include "PluginManager.h"
 
 #include "modules/Gui.h"
@@ -174,7 +175,13 @@ command_result df_strangemood (color_ostream &out, vector <string> & parameters)
                 out.printerr("No unit id specified!\n");
                 return CR_WRONG_USAGE;
             }
-            unit = df::unit::find(std::stoi(parameters[i]));
+            int id = string_to_int(parameters[i], -1);
+            if (id < 0)
+            {
+                out.printerr("Invalid unit id: '{}'\n", parameters[i]);
+                return CR_WRONG_USAGE;
+            }
+            unit = df::unit::find(id);
             if (!unit)
                 return CR_FAILURE;
         }

plugins/suspendmanager.cpp

@@ -890,7 +890,7 @@ static command_result do_command(color_ostream &out, vector<string> &parameters)
         return plugin_enable(out,true);
     } else if (parameters[0] == "disable") {
         return plugin_enable(out,false);
-    } else if (parameters[0] == "set" && parameters[1] == "preventblocking") {
+    } else if (parameters.size() == 3 && parameters[0] == "set" && parameters[1] == "preventblocking") {
         if (parameters[2] == "true") {
             suspendmanager_instance->prevent_blocking = true;
             config.set_bool(CONFIG_PREVENT_BLOCKING, true);

test/plugins/3dveins.lua

@@ -0,0 +1,18 @@
+config.mode = 'fortress'
+config.target = '3dveins'
+
+function test.rewrites_veins()
+    local output, status = dfhack.run_command_silent('3dveins')
+    expect.eq(CR_OK, status)
+    expect.str_find('Writing tiles', output)
+end
+
+function test.verbose_option()
+    local _, status = dfhack.run_command_silent('3dveins', 'verbose')
+    expect.eq(CR_OK, status)
+end
+
+function test.bad_option_is_wrong_usage()
+    local _, status = dfhack.run_command_silent('3dveins', 'bogus')
+    expect.eq(CR_WRONG_USAGE, status)
+end

test/plugins/autodump.lua

@@ -0,0 +1,147 @@
+config.mode = 'fortress'
+config.target = 'autodump'
+
+local dwarfmode = require('gui.dwarfmode')
+
+local function find_floor_pos()
+    for _, block in ipairs(df.global.world.map.map_blocks) do
+        for x = 0, 15 do
+            for y = 0, 15 do
+                local tt = block.tiletype[x][y]
+                local des = block.designation[x][y]
+                local occ = block.occupancy[x][y]
+                if df.tiletype.attrs[tt].shape == df.tiletype_shape.FLOOR
+                        and not des.hidden and occ.building == 0 then
+                    return block.map_pos.x + x, block.map_pos.y + y,
+                           block.map_pos.z
+                end
+            end
+        end
+    end
+end
+
+local function set_cursor(x, y, z)
+    dwarfmode.setCursorPos(xyz2pos(x, y, z))
+end
+
+local function clear_cursor()
+    dwarfmode.clearCursorPos()
+end
+
+local function find_dumpable_item()
+    for _, item in ipairs(df.global.world.items.other.IN_PLAY) do
+        if not item.flags.dump and not item.flags.construction
+                and not item.flags.in_building and not item.flags.artifact
+                and not item.flags.in_job and not item.flags.forbid
+                and not item.flags.owned and not item.flags.in_inventory then
+            return item
+        end
+    end
+end
+
+local function block_has_item(block, id)
+    for _, bid in ipairs(block.items) do
+        if bid == id then return true end
+    end
+    return false
+end
+
+local function items_sorted(block)
+    local prev = -1
+    for _, id in ipairs(block.items) do
+        if id <= prev then return false end
+        prev = id
+    end
+    return true
+end
+
+function test.dump_moves_item_to_cursor()
+    local x, y, z = find_floor_pos()
+    local item = find_dumpable_item()
+    expect.ne(nil, x, 'test needs a revealed floor tile')
+    expect.ne(nil, item, 'test needs a dumpable item')
+    local ix, iy, iz = dfhack.items.getPosition(item)
+    expect.ne(nil, ix, 'test item needs a real position')
+    local old_block = dfhack.maps.getTileBlock(ix, iy, iz)
+    local new_block = dfhack.maps.getTileBlock(x, y, z)
+
+    return dfhack.with_finalize(function()
+        item.flags.dump = false
+        clear_cursor()
+    end, function()
+        set_cursor(x, y, z)
+        item.flags.dump = true
+
+        local _, status = dfhack.run_command_silent('autodump')
+        expect.eq(CR_OK, status)
+        -- item was teleported to the cursor and marked as dumped
+        expect.eq(x, item.pos.x)
+        expect.eq(y, item.pos.y)
+        expect.eq(z, item.pos.z)
+        expect.false_(item.flags.dump)
+        expect.true_(item.flags.forbid)
+        expect.true_(item.flags.on_ground)
+        expect.false_(item.flags.in_inventory)
+        expect.false_(item.flags.in_building)
+
+        -- the destination tile's occupancy and the map_block item
+        -- vectors must reflect the move
+        local _, occ = dfhack.maps.getTileFlags(x, y, z)
+        expect.true_(occ.item)
+        expect.true_(block_has_item(new_block, item.id),
+            'item id should be in the new map_block.items')
+        if old_block ~= new_block then
+            expect.false_(block_has_item(old_block, item.id),
+                'item id should be removed from the old map_block.items')
+        end
+        expect.true_(items_sorted(new_block),
+            'map_block.items should remain sorted')
+    end)
+end
+
+function test.conflicting_filters_is_wrong_usage()
+    local output, status = dfhack.run_command_silent('autodump', 'visible', 'hidden')
+    expect.eq(CR_WRONG_USAGE, status)
+    expect.str_find("both hidden and visible", output)
+end
+
+function test.bad_option_is_wrong_usage()
+    local _, status = dfhack.run_command_silent('autodump', 'bogus')
+    expect.eq(CR_WRONG_USAGE, status)
+end
+
+function test.destroy_and_undestroy()
+    local item = find_dumpable_item()
+    expect.ne(nil, item, 'test needs a dumpable item')
+
+    local was_paused = dfhack.world.ReadPauseState()
+    return dfhack.with_finalize(function()
+        dfhack.world.SetPauseState(was_paused)
+        dfhack.run_command_silent('autodump', 'undestroy')
+        item.flags.dump = false
+        item.flags.garbage_collect = false
+        item.flags.forbid = false
+        item.flags.hidden = false
+    end, function()
+        item.flags.dump = true
+        -- undestroy only restores marks made in the same frame, so the
+        -- game must stay paused between destroy and undestroy
+        dfhack.world.SetPauseState(true)
+
+        local _, status = dfhack.run_command_silent('autodump', 'destroy')
+        expect.eq(CR_OK, status)
+        expect.true_(item.flags.garbage_collect)
+        expect.true_(item.flags.forbid)
+        expect.true_(item.flags.hidden)
+
+        local output, status2 = dfhack.run_command_silent('autodump', 'undestroy')
+        expect.eq(CR_OK, status2)
+        expect.str_find('unmarked for destruction', output)
+        expect.false_(item.flags.garbage_collect)
+        expect.false_(item.flags.forbid)
+        expect.false_(item.flags.hidden)
+        -- undestroy restores the pre-destroy flags, including the dump
+        -- flag we set ourselves
+        expect.true_(item.flags.dump)
+    end)
+end

test/plugins/changeitem.lua

@@ -0,0 +1,85 @@
+config.mode = 'fortress'
+config.target = 'changeitem'
+
+local dwarfmode = require('gui.dwarfmode')
+
+local function find_floor_item_pos()
+    for _, item in ipairs(df.global.world.items.other.IN_PLAY) do
+        if not item.flags.hidden and not item.flags.in_inventory
+                and not item.flags.in_job and not item.flags.construction then
+            local x, y, z = dfhack.items.getPosition(item)
+            if x then return item, x, y, z end
+        end
+    end
+end
+
+local function set_cursor(x, y, z)
+    dwarfmode.setCursorPos(xyz2pos(x, y, z))
+end
+
+local function clear_cursor()
+    dwarfmode.clearCursorPos()
+end
+
+function test.here_quality_changes_item()
+    local item, x, y, z = find_floor_item_pos()
+    expect.ne(nil, item, 'test needs an item on the ground')
+    local orig_quality = item.quality
+
+    return dfhack.with_finalize(function()
+        item.quality = orig_quality
+        clear_cursor()
+    end, function()
+        set_cursor(x, y, z)
+        local output, status = dfhack.run_command_silent('changeitem',
+            'here', 'q', '4')
+        expect.eq(CR_OK, status)
+        expect.str_find('items processed', output)
+        expect.eq(4, item.quality)
+    end)
+end
+
+function test.here_empty_tile_processes_nothing()
+    -- find a floor tile and put the cursor on it; if it happens to hold
+    -- items the count is still reported
+    local block = df.global.world.map.map_blocks[0]
+    local x, y, z = block.map_pos.x, block.map_pos.y, block.map_pos.z
+
+    return dfhack.with_finalize(clear_cursor, function()
+        set_cursor(x, y, z)
+        local output, status = dfhack.run_command_silent('changeitem', 'here')
+        expect.eq(CR_OK, status)
+        expect.str_find('items processed', output)
+    end)
+end
+
+function test.here_no_cursor_is_failure()
+    return dfhack.with_finalize(clear_cursor, function()
+        clear_cursor()
+        local output, status = dfhack.run_command_silent('changeitem', 'here')
+        expect.eq(CR_FAILURE, status)
+        expect.str_find('Cursor position not found', output)
+    end)
+end
+
+function test.no_selection_is_failure()
+    local output, status = dfhack.run_command_silent('changeitem')
+    expect.eq(CR_FAILURE, status)
+    expect.str_find('No item selected', output)
+end
+
+function test.material_missing_arg_is_wrong_usage()
+    local output, status = dfhack.run_command_silent('changeitem', 'm')
+    expect.eq(CR_WRONG_USAGE, status)
+    expect.str_find('no material specified', output)
+end
+
+function test.bad_quality_is_wrong_usage()
+    local _, status = dfhack.run_command_silent('changeitem', 'q', '9')
+    expect.eq(CR_WRONG_USAGE, status)
+end
+
+function test.bad_option_is_wrong_usage()
+    local _, status = dfhack.run_command_silent('changeitem', 'bogus')
+    expect.eq(CR_WRONG_USAGE, status)
+end

test/plugins/changelayer.lua

@@ -0,0 +1,33 @@
+config.mode = 'fortress'
+config.target = 'changelayer'
+
+local dwarfmode = require('gui.dwarfmode')
+
+function test.no_material_is_wrong_usage()
+    local output, status = dfhack.run_command_silent('changelayer')
+    expect.eq(CR_WRONG_USAGE, status)
+    expect.str_find('specify a material', output)
+end
+
+function test.bad_material_is_failure()
+    local output, status = dfhack.run_command_silent('changelayer', 'BOGUSMAT')
+    expect.eq(CR_FAILURE, status)
+    expect.str_find('No such material', output)
+end
+
+function test.no_cursor_is_failure()
+    return dfhack.with_finalize(function()
+        dwarfmode.clearCursorPos()
+    end, function()
+        dwarfmode.clearCursorPos()
+        local output, status = dfhack.run_command_silent('changelayer',
+            'GRANITE')
+        expect.eq(CR_FAILURE, status)
+        expect.str_find('No cursor', output)
+    end)
+end
+
+function test.help_is_wrong_usage()
+    local _, status = dfhack.run_command_silent('changelayer', '?')
+    expect.eq(CR_WRONG_USAGE, status)
+end