fix(bvm): SCENERYOWNER stack-imbalance — push 0 sentinel; document dead-BVM trap#297
Merged
Conversation
…ad-BVM trap
The SCENERYOWNER dispatch case in RC_Standard_Invoker.bb at Case 501
popped 3 args from the BVM stack but did not push anything (the
;BVM_PushInt(BVM_SCENERYOWNER(...)) call was commented out alongside
the impl when the OwnedScenery type was disabled in ServerAreas.bb:53).
SCENERYOWNER's contract declares it as a `%`-returning function -- the
BlitzForge compiler emits the call expecting one int on the stack
afterwards. With the case popping 3 and pushing 0, the caller's
subsequent BVM operation pops the wrong slot, silently corrupting the
expression. This is a real bug, not a hypothetical -- any script that
ever called `SceneryOwner(zone, id)` would produce stack-shifted
expression results from that point until the next stack reset.
Fix: add `BVM_PushInt(0)` to Case 501 so the caller's stack stays
balanced. 0 reads as "no owner" -- the closest defined sentinel given
that the underlying OwnedScenery storage is gone.
Sibling Case 530 (SETOWNER) is void, so its 4-arg pop self-balances
and remains a silent no-op. Added an audit comment recording that.
Also added audit comments at all FIVE dead-API sites:
1. RC_Standard_Invoker.bb:~95 -- contract entries for SETOWNER /
SCENERYOWNER (cannot be removed without renumbering every BVM
opcode alphabetically after them and breaking the fixed-Case
dispatch at 1363 / 1494).
2. RC_Standard_Invoker.bb:501 -- SCENERYOWNER no-op-with-sentinel.
3. RC_Standard_Invoker.bb:530 -- SETOWNER no-op.
4. ScriptingCommands.bb:1046 -- commented BVM_SETOWNER impl.
5. ScriptingCommands.bb:1071 -- commented BVM_SCENERYOWNER impl.
The audit-comment cluster cross-links the 5 sites and the disabled
OwnedScenery Type, so the next contributor who encounters one of them
has the full story without grepping.
Recon was a sweep of every <BVM_*> contract entry against `^Function
BVM_*` impls (220 contract vs 222 impl, case-insensitive comm). Only
these two were contract-without-impl orphans; the 4 impl-without-
contract entries (BVM_RequirePrivileged, RequireSelfOrPrivileged,
ScriptPathIsSafe, SetWaitResult) are correctly-private helpers.
Compile-verified clean across Server/Client/GUE/Project Manager.
All 26 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer corrections on the dead-BVM audit-comment cluster:
1. Stale line numbers (3 of 5 audit cross-references were off after the
comment blocks themselves displaced the dispatch cases downward).
Rewrote as symbolic references:
- "ServerAreas.bb:53" -> "ServerAreas.bb (the Type OwnedScenery and
Field OwnedScenery[] declarations are commented out)" -- the Type
name is unique enough to grep.
- "RC_Standard_Invoker.bb:1363/1494" -> "Case 501 / Case 530" --
Case numbers don't drift even if the surrounding code shifts.
- "ScriptingCommands.bb:1046/1071" -> the impls are now found via
`grep ';Function BVM_SETOWNER'` / `;Function BVM_SCENERYOWNER`
instead of by line.
- "RC_Standard_Invoker.bb:~95" -> "near the top of this file (grep
'DEAD-API')" -- the unique sentinel finds the comment.
2. "Above" wording was wrong (the contract entries are *after* the
audit-comment block, not before). Replaced with "next two lines".
3. .bcs file was not updated. Added the same audit-comment block at
src/RC_Standard.bcs:113-121 so a maintainer reading the static
command-set file sees the same trap warning as one reading the
runtime-string builder.
Recon also surfaced that the just-rebased develop pulled in PR #292
(Loom Alpha) which adds src/Loom.bb + src/Modules/Loom/Theme.bb.
Verified Loom compiles cleanly alongside the BVM fix.
Compile-verified across all 5 engine targets (Server + Client + GUE +
Project Manager + Loom).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CoreyRDean
force-pushed
the
fix/dead-bvm-api
branch
from
3479fea to
13cef2d
Compare
5 tasks
CoreyRDean
added a commit
that referenced
this pull request
May 27, 2026
Pre-commit verification (the same symbolic-refs lesson from PR #297): two of the cited line numbers for the importance check were off: ServerNet.bb:353 should be :356, ServerNet.bb:367 should be :370. Replaced the line-number list with symbolic references (function / case names) per the doc-audit lesson. Also adjusted the prose: noted there are SIX sites (not five) -- I under-counted by lumping the two ServerNet handlers as one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
CoreyRDean
added a commit
that referenced
this pull request
May 27, 2026
docs/modules/scripting.md (the already-existing runtime-half overview) references docs/modules/scriptingcommands.md as the implementation- half companion -- but the file didn't exist. Dangling link surfaced during iteration #38 recon. Wrote the module-level overview for ScriptingCommands.bb (~3300 lines, 222 BVM_* functions): * File-structure section table (~50-3300 grouped by theme: privilege helpers / actor lifecycle / items / spells / attributes / party / output / persistence / UDP). Refresh trigger when reorganization happens; navigation aid, not strict spec. * Privilege gating section consolidates the four CLAUDE.md gate categories, the clicker-handle trap (`SI\AI = Handle(clicker)` for Examine/Trade/RightClick/ItemScript), and the full currently-gated brick-vector cluster (11 functions with their threat shapes). * Dead-API surface (BVM_SETOWNER + BVM_SCENERYOWNER) -- references PR #297's stack-balance sentinel fix and the opcode-stability rationale for keeping the contract entries alive. * Float / integer hardening overview -- ClampWorldCoord# / ClampSaneFloat# and the bounds-check-before-array-index pattern. * Handle-Null discipline -- the canonical entry pattern that every BVM body must follow (Object.X(handle) returns Null for stale handles). * "Adding a new BVM function" three-file-lockstep procedure (ScriptingCommands.bb impl + RC_Standard_Invoker.bb contract + dispatch Case + RC_Standard.bcs compile-time twin), the alphabetical-opcode-shift gotcha, and the privilege-gate decision tree. * Notable historical hardening table cross-referencing PRs #260, #237-#239, #246-#248, #233/#234, #300, #301, #304. * Related-modules section linking back to scripting.md / bvm-reference.md / RC_Standard_Invoker.bb / RC_Standard.bcs / ServerNet.bb / BVMPrivilegeGateTest.bb / CLAUDE.md. Closes the dangling-link gap; future scripting.md readers now have a landing page for the implementation half. rc_standard_invoker.md is still a dangling link from scripting.md -- deferred to a separate iteration (it's more arcane; the BVM-reference auto-gen already covers the user-facing API surface). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applied the just-learned doc-audits-find-source-bugs recon pattern to a sibling area: the invoker-contract vs. BVM-impl drift. There's known precedent — the audit-comment at ScriptingCommands.bb:2780 records that
BVM_OUTPUT'sParam3 = 0impl default mismatched the invoker contract's255, causing every 2-argOutput(player, \"hi\")call to print black-on-black. If one was wrong, others likely were too.Sweep: 220 contract entries (
<BVM_*>inRC_Standard_Invoker.bb) vs. 222 active impls (^Function BVM_*inScriptingCommands.bb), case-insensitive. Two contract-without-impl orphans:BVM_SETOWNERandBVM_SCENERYOWNER. The 4 impl-without-contract entries (RequirePrivileged/RequireSelfOrPrivileged/ScriptPathIsSafe/SetWaitResult) are correctly-private internal helpers.The bug
The underlying
OwnedSceneryType was disabled long ago in ServerAreas.bb:53, and both BVM impls were commented out in ScriptingCommands.bb:1046 / :1071. The dispatch cases in RC_Standard_Invoker.bb:1363 / :1494 were also commented out. But the public contract entries at lines ~95-96 were left active, so the BlitzForge compiler still acceptsSetOwner(...)/SceneryOwner(...)calls from scripts.The dispatch cases pop args from the BVM stack but the (commented-out) impl calls never happen:
%)So SETOWNER is silently no-op (annoying but safe). SCENERYOWNER is a real silent-corruption bug: the caller's expression expects an int on the stack after the call, gets a stack underflow instead, and every subsequent BVM operation in that expression pops the wrong slot.
Fix
RC_Standard_Invoker.bb:501— pushBVM_PushInt(0)so SCENERYOWNER's stack stays balanced. 0 reads as "no owner" — the closest defined sentinel given the storage is gone.RC_Standard_Invoker.bb:~95: documenting why they stay alive (removing would renumber every opcode alphabetically after them and break the fixed-Case dispatch).ScriptingCommands.bb:1046and:1071.Next contributor who hits any one of these has the full story without needing to grep the other four.
Why not remove the contract entries
The BlitzForge command-set parser assigns opcodes alphabetically by function name. Removing SCENERYOWNER and SETOWNER would shift every BVM alphabetically after them down by 2, breaking the fixed-Case dispatch at lines 501+ for SCREENFLASH / SCRIPTLOG / SETACTORX / ... / WARP. That's a ~250-Case renumber-or-mapping audit — risky for a doc-only improvement. Audit comments + the sentinel fix is the surgical-safe choice.
Test plan
compile.bat -tclean (Server + Client + GUE + Project Manager)test.bat— all 26 tests passdata/reference SetOwner/SceneryOwner (grep clean)🤖 Generated with Claude Code