fix three plugin crash/UB bugs; test coverage for 14 plugins by Alistair-Afton · Pull Request #5939 · DFHack/dfhack

Alistair-Afton · 2026-09-17T15:33:27Z

Summary

3dveins: fix a crash when a tile's biome cannot be resolved (biome_by_idx.at(-1) threw std::out_of_range, which escaped the command handler and terminated the game)
strangemood: fix a crash when --id is given a non-numeric value (std::stoi threw uncaught); invalid ids now return a usage error
suspendmanager: fix an out-of-bounds parameter read when set is given too few arguments
New in-engine regression tests for 3dveins, autodump, changeitem, changelayer, createitem, deramp, export-world-map, filltraffic, flows, liquids, showmood, strangemood, suspendmanager, and tubefill (57 tests)

Test plan

All new tests pass in-game
Full test suite passes (329/330; the one failure was a scratch file not part of this change)

Alistair-Afton · 2026-09-17T17:59:40Z

Root cause of the recurring scripts/overlay/notes:render_existing_notes failure found: NotesOverlay:onRenderFrame() skips drawing map pins while the game is unpaused (in text mode), and the test relies on the ambient pause state set by ensure_fortress. Two tests in this batch (autodump, liquids) forced pause_state = false in their finalizers instead of restoring the prior state, leaving the game unpaused for the notes suite.

Fixed in f42685d: both tests now save/restore pause_state, and suspendmanager's test restores the persisted preventblocking value instead of hardcoding it. Also opened DFHack/scripts#1632 to make the notes test set/restore pause itself so it's robust against similar leaks elsewhere.

SilasD

this is not a full review, I will do that on another day as I am running out of steam today.

in general I don't care about correct code in test suites nearly as much as correct code in tools.

SilasD · 2026-09-17T18:45:58Z

+    local was_paused = df.global.pause_state
+    return dfhack.with_finalize(function()
+        df.global.pause_state = was_paused


please use the existing API:

dfhack.world.ReadPauseState()
Returns true if the game is paused.

dfhack.world.SetPauseState(paused)
Sets the pause state of the game.

SilasD · 2026-09-17T18:46:52Z

+        item.flags.dump = true
+        -- undestroy only restores marks made in the same frame, so the
+        -- game must stay paused between destroy and undestroy
+        df.global.pause_state = true


and API here.

SilasD · 2026-09-17T18:47:32Z

+    local was_paused = df.global.pause_state
+    return dfhack.with_finalize(function()
+        local des = des_at(x, y, z)
+        des.flow_size = 0
+        df.global.pause_state = was_paused
+        clear_cursor()
+    end, function()
+        -- pause so the magma cannot spread before we clean it up
+        df.global.pause_state = true


and existing pause API here.

std::stoi throws uncaught on non-numeric input, crashing the game. Use string_to_int and reject invalid ids with CR_WRONG_USAGE.

…gemood

parameters[1] was read unconditionally when the first parameter was 'set', which is undefined behavior when fewer arguments are given.

…eateitem

MapExtras::Block::biomeIndexAt returns -1 when the biome lookup fails, and biome_by_idx.at(-1) threw std::out_of_range. The exception escaped the plugin command handler and terminated the game. Return NULL like the existing unresolvable-layer path.

Tests must not leak global state to subsequent tests. Restore the prior pause_state instead of forcing unpaused, and restore suspendmanager's persisted preventblocking value instead of hardcoding it.

Trigger real fey/fell/macabre moods per skill and assert the generated job_items match the documented vanilla sets, that BAR and CLOTH demands carry the dimension-corrected quantities (the plugin's vanilla bugfix), and that thread is never requested.

The fallback race lookup dereferenced civ without a null check, so a site whose owner has an undefined race and whose civ_id does not resolve crashed DF (observed as an access violation in CI on the Windows empty-config leg). Assign the civ's race as the intended fallback and only warn when the owner's race is actually undefined.

Alistair-Afton · 2026-09-17T22:53:04Z

The Windows empty-config leg failure is root-caused and fixed in a38d479: export-world-map sites (covered by the new test) dereferenced a null civ when a site's owner had an undefined race — a latent crash in develop that no existing test exercised. DF was crashing mid-test (access violation), which the harness records as a failed test; the heap-corruption exit on the second DF instance is the usual quit-time noise.

Fixed by guarding the null civ and actually assigning the intended civ-race fallback. Verified in-game: export-world-map sites exports cleanly and both tests in test/plugins/export-world-map.lua pass.

SilasD · 2026-09-17T23:29:20Z

+local function set_cursor(x, y, z)
+    df.global.cursor:assign{x=x, y=y, z=z}
+end
+
+local function clear_cursor()
+    df.global.cursor:assign{x=-30000, y=-30000, z=-30000}
+end


we have a getter, a setter, and a clearer for these, not in the core API but in a rather obscure place:
local dwarfmode = require('gui.dwarfmode') -- near the top of the script
printall(dwarfmode.getCursorPos()) -- returns nil on invalid cursor.
dwarfmode.setCursorPos( xyz2pos(1,2,3) )
dwarfmode.clearCursorPos()

OTOH this is just a test harness. I won't insist that you use them.

also that's a very old module; much of it became obsolete with the move to 0.50+. all of the Viewport class and all of the DwarfOverlay class are obsolete and have been replaced with the Widgets hierarchy, everything related to the Sidebar is obsolete and definately nonfunctional, I wouldn't trust MOVEMENT_KEYS to still be valid.

dead code everywhere. this needs a weed-whacker. I wonder if any of that stuff is still referenced anywhere else.

the Cursor functions still work, the Selection functions might still work.

update or not, as you choose. I'll do a full review another day; my eyes are blurring.

Alistair-Afton force-pushed the test-plugin-coverage-10 branch 2 times, most recently from f3c1610 to 6e64b3a Compare September 17, 2026 15:50

Alistair-Afton changed the title ~~fix strangemood --id crash; test coverage for autodump, filltraffic, export-world-map, strangemood~~ fix strangemood/suspendmanager input validation; test coverage for 9 more plugins Sep 17, 2026

Alistair-Afton changed the title ~~fix strangemood/suspendmanager input validation; test coverage for 9 more plugins~~ fix strangemood/suspendmanager input validation; test coverage for 14 more plugins Sep 17, 2026

Alistair-Afton changed the title ~~fix strangemood/suspendmanager input validation; test coverage for 14 more plugins~~ fix three plugin crash/UB bugs; test coverage for 14 plugins Sep 17, 2026

Alistair-Afton mentioned this pull request Sep 17, 2026

core: report plugin command exceptions instead of crashing the game #5940

Open

3 tasks

Alistair-Afton force-pushed the test-plugin-coverage-10 branch from 583dfb2 to 14ccd5b Compare September 17, 2026 17:22

Alistair-Afton mentioned this pull request Sep 17, 2026

notes test: pause the game while the overlay is installed DFHack/scripts#1632

Open

1 task

SilasD requested changes Sep 17, 2026

View reviewed changes

Alistair-Afton added 8 commits September 17, 2026 23:36

fix strangemood crash on non-numeric --id value

edf63fd

std::stoi throws uncaught on non-numeric input, crashing the game. Use string_to_int and reject invalid ids with CR_WRONG_USAGE.

test: add coverage for autodump, filltraffic, export-world-map, stran…

60b62c3

…gemood

suspendmanager: fix out-of-bounds read on bare 'set'

5cd9f4c

parameters[1] was read unconditionally when the first parameter was 'set', which is undefined behavior when fewer arguments are given.

test: add coverage for liquids, tubefill, suspendmanager, 3dveins, cr…

8617b23

…eateitem

test: add coverage for flows, showmood, deramp, changelayer, changeitem

cac3cd9

restore global state in test cleanup

327f4bc

Tests must not leak global state to subsequent tests. Restore the prior pause_state instead of forcing unpaused, and restore suspendmanager's persisted preventblocking value instead of hardcoding it.

use dfhack.world pause API instead of raw pause_state

610c05d

Alistair-Afton force-pushed the test-plugin-coverage-10 branch from 934c663 to 610c05d Compare September 17, 2026 21:36

Alistair-Afton added 2 commits September 17, 2026 23:57

SilasD reviewed Sep 17, 2026

View reviewed changes

Alistair-Afton added 2 commits September 18, 2026 01:54

strangemood test: mirror STRANGE_MOODS caste check in unit picker

00773e5

autodump test: use gui.dwarfmode cursor helpers

e672c73

Navigation Menu

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

fix three plugin crash/UB bugs; test coverage for 14 plugins#5939
Alistair-Afton wants to merge 12 commits into
DFHack:developfrom
Alistair-Afton:test-plugin-coverage-10

Alistair-Afton commented Sep 17, 2026 •

edited

Loading

Uh oh!

Alistair-Afton commented Sep 17, 2026

Uh oh!

SilasD left a comment

Uh oh!

SilasD Sep 17, 2026

Uh oh!

SilasD Sep 17, 2026

Uh oh!

SilasD Sep 17, 2026

Uh oh!

Alistair-Afton commented Sep 17, 2026

Uh oh!

SilasD Sep 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Alistair-Afton commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Alistair-Afton commented Sep 17, 2026

Uh oh!

SilasD left a comment

Choose a reason for hiding this comment

Uh oh!

SilasD Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

SilasD Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

SilasD Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

Alistair-Afton commented Sep 17, 2026

Uh oh!

SilasD Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Alistair-Afton commented Sep 17, 2026 •

edited

Loading