do_get reads past the end of "all": get all / autoloot crash debug builds, skip the undroppable guard in release

Written by whoever made the change (here, Claude Code working from the dossier), answering the dossier's questions. The facts it cites are in dossier.md; the proof is in gate.md.

In one paragraph

do_get decides between get all and get all.<name> by looking at arg1[3], the character after "all". That worked when arg1 was a NUL-terminated char[]: arg1[3] was the terminator or the dot. In December 2021 arg1 became a std::string_view, which has no terminator, so for the 3-character view "all" the read is past the end: undefined behaviour. Release builds read the next byte of the command line (a space, in get all corpse), so the guard that keeps autoloot from taking undroppable items has never worked in release since then. Debug builds didn't notice either, until the project moved to GCC 15 on 2026-10-31, whose standard library checks operator[] in unoptimised builds: from then on every autoloot of a non-empty corpse aborts a debug MUD. The fix decides "all" or "all.name" once, as named booleans, and passes the intent to the guard instead of a character.

Root cause

The timeline, from section 8 of the dossier and the commits it points at:

Why debug builds abort and release builds don't

Checked directly with g++ 15.2 on a 10-line program that indexes std::string_view("all corpse", 3) at 3:

So it is undefined behaviour in every build, as suspected; the assertion only makes it visible. The /usr/include/x86_64-linux-gnu/c++/15/bits/c++config.h lines that do it:

# elif ! defined(__OPTIMIZE__) && ! defined(_GLIBCXX_NO_ASSERTIONS)
// Enable assertions for unoptimized builds.
#  define _GLIBCXX_ASSERTIONS 1

What each faulty line did, in release

• 707, get all corpse and autoloot: the guard got ' ' instead of '\0', so it never refused anything. Every NODROP and NOREMOVE item on an NPC corpse was looted. The gate's before/after table shows it: on the base, in Release, the new test finds the ring and the axe in the player's inventory.
• 707, get all.<name> corpse: by then arg1 has had "all." removed, so arg1[3] was the 4th character of the name. For names of 4 or more characters that's not '\0', so named looting was allowed (by luck). For shorter names (get all.bo corpse) it read past the end again. The issue missed this one; the dossier's detector found it.
• 641, 648, 651, get all / get all.<name> in a room: these read the byte after the view; they worked when it happened to be a NUL (a command typed alone on the line), and &arg1[4] was a const char* read up to the next NUL.

The fix

// "all" takes everything; "all.<name>" takes everything called <name>, and arg1 becomes just the <name>.
const auto is_all = matches(arg1, "all");
const auto is_all_named = matches_start("all.", arg1);
if (is_all_named)
    arg1.remove_prefix(4);

hoisted above the room/container split, so both branches use the same two booleans; the room branch then uses is_all and arg1 (the name) exactly as the container branch already did after 35c8f3a. The guard takes the intent instead of a character:

bool is_mass_looting_npc_undroppable_obj(const Object *obj, const Object *container, const bool is_unnamed_all)
...
if (is_mass_looting_npc_undroppable_obj(obj, container, is_all)) {

That finishes the conversion 35c8f3a started, in the idioms it chose (section 3 of the dossier: do_get already had is_all = matches(arg1, "all") and is_all_named = matches_start("all.", arg1); the fix moves them up rather than adding a third way). src/act_obj.cpp changes by 14 lines added and 13 removed, two of the added lines being comments that say what "all" and the guard mean.

Alternatives considered

• Bounds-checked reads (arg1.size() > 3 && arg1[3] == '.'): keeps the C-string reasoning that caused the bug, and leaves three different ways of asking "is it all?" in one function. Rejected.
• Only fix line 707 (the crash Sim found first): leaves the empty-room crash (648) and the short-name read. Rejected; the detector lists all five, and they're the same mistake.
• Disable the libstdc++ assertions (-D_GLIBCXX_NO_ASSERTIONS): hides the symptom, keeps the UB and the dead guard. The assertions are doing their job; nobody should do this.
• Rewrite do_get (115 lines, the dossier flags it as long): out of scope for a bug fix; would make review harder.

Answers to the dossier's questions

1. Explorer. No Explorer check covers this. One is proposed (code.get-all-past-end) so Explorer's Bugs page shows it as fixed once this lands; it isn't part of this PR (Explorer is a separate tool).

2. Architecture. The change is local to do_get and its file-local helper. The only automatic caller, loot_and_sacrifice_corpse (autoloot), passes "all corpse" and now gets the behaviour 5e72201 intended. No other caller of the helper exists.

3. Design patterns. It follows the idiom already in the function (named booleans from matches / matches_start, remove_prefix). No new helper was needed.

4. Technical debt. Pays down all five C-string reads in the focus (the gate's detector step: 5 before, 0 after) and removes a duplicated is_all computation. Leaves do_get's length alone. The same pattern elsewhere in src/ (dossier section 4), place by place:

• MudImpl::nanny (6 places): one is guarded (GetAnsi); the other five are real, filed as #3, separate PR.
• do_who arg[0], do_permit player[0]: guarded (an empty() check comes first); false positives. The detector should learn about guards.
• skill_lookup name[0]: an unverified lead. do_practice passes args.shift() after checking only args.empty(); if practice '' at a trainer shifts an empty view (ArgParser strips quotes), it reads past the end. Worth a Sim check and its own issue; not in this change.

5. Gameplay. Debug builds: no more aborts on get all or autoloot. Release builds (the live game): players will notice a change: get all corpse and autoloot now leave NODROP/NOREMOVE items on NPC corpses, with "You refrain from looting $p as it appears to be undroppable. You can still 'get' it using its name.", as the original feature meant to. Named looting (get ring corpse, get all.ring corpse) still takes them. Nothing to abuse: the change only takes away an unintended pickup. Explorer's balance models don't read act_obj.cpp.

6. Documentation. The help entry DROP GET GIVE PUT TAKE doesn't describe the guard; the in-game message explains it when it happens. No help change. The PR says the release behaviour changes.

7. Tests. New src/test/GetTest.cpp, 2 test cases and 10 sections, driving do_get itself on MockMud: empty room (all, all.sword, all.), picking up, the corpse guard (NODROP, NOREMOVE, a plain item), named looting, a short name, and a container (not a corpse) that isn't guarded. On the base they abort the Debug build and fail in Release (the gate's before/after table); on the branch they pass in both. From the player's side: Sim's known-bug scenario (a bot types get all in an empty room) crashes the base and no longer crashes the branch, and a 5-minute hunt (four bots killing cave beasts with autoloot on) runs clean on the branch. The hunt isn't used as proof on the base: the guard's read only happens for a corpse with something in it, and cave beasts rarely carry anything (the first gate run saw 16 kills on the base without a crash). The corpse path's proof is the unit test.

8. History. Answered under Root cause. Upstream has no issue or PR about it (the search finds only upstream #72, a different autoloot bug, closed).

Follow-ups (not in this PR)

• Xania Sim: retire test_get_all_in_an_empty_room_crashes_debug_builds once this is merged (it asks to be).
• Xania Explorer: the code.get-all-past-end check.
• Builder: teach the string_view detector about empty() guards (2 false positives here).
• Check the practice '' lead with Sim.

Branch fix/1-do-get-all at 515b3001ab against base 82171fa443: passed in 7 min, 2026-11-11 10:56.

Environment: gcc g++-15 (Ubuntu 15.2.0-16ubuntu1) 15.2.0; cmake cmake version 4.2.3; os Ubuntu 26.04.1 LTS; distro Ubuntu-26.04

Changes: 2 files changed, 170 insertions(+), 13 deletions(-)

• 515b300 GetTest: a Scene fixture so ~Char doesn't extract stack objects
• 73188fc GetTest: allow the mock's logger
• 65f1f6e GetTest: test the loot guard through do_get (the helper is file-local)
• a5aeef1 GetTest: Treasure, not Jewelry
• 8e58a8d do_get: decide 'all' vs 'all.<name>' once instead of indexing past the end (#1)

The new tests, before and after

Detectors, base against branch

Logs

/usr/include/c++/15/string_view:260: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<char>; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed.

smoke: Smoke: create a bot, look, quit (21 s, Xania 515b3001ab)
  transcript: runs\20261111-105703\Smokea.log
  result: clean

.                                                                        [100%]
1 passed, 1 deselected in 38.81s

F                                                                        [100%]
================================== FAILURES ===================================
_____________ test_get_all_in_an_empty_room_crashes_debug_builds ______________
mud = <xania_sim.core.server.TestMud object at 0x000001957D641550>
names = <function names.<locals>.<lambda> at 0x000001957F65C3B0>, found = []
    async def test_get_all_in_an_empty_room_crashes_debug_builds(mud, names, found):
        """act_obj.cpp do_get: 'get all' reads arg1[3] where arg1 is "all" (3 chars) when nothing is found (line 648).
        Each run that still sees it adds an occurrence to the finding (so the dispatch shows it's still there)."""
        bot = Bot(names(), "simpass1", host=mud.host, port=mud.port)
        await bot.enter(Profile())
        crashed = await _crashes_after(mud, bot, "get all")
        report = mud.crash_report() if crashed else ""
        await bot.close()
        if crashed:
            found.append(crash_finding(report, 'python -m pytest scenarios/test_live_bugs.py -k get_all',
                                       "A bot typed 'get all' in an empty room (a known-bug check)."))
            await mud.restart()
>       assert crashed, "'get all' no longer crashes xania: the bug looks fixed, retire this test"
E       AssertionError: 'get all' no longer crashes xania: the bug looks fixed, retire this test
E       assert False
scenarios\test_live_bugs.py:41: AssertionError
=========================== short test summary info ===========================
FAILED scenarios/test_live_bugs.py::test_get_all_in_an_empty_room_crashes_debug_builds
1 failed, 1 deselected in 34.89s

hunt: Hunt: angry cave beast, 4 bots (308 s, Xania 515b3001ab)
  fights 16, bot errors 0, crashed False
  result: clean

#1 · Xania 82171fa443 · 2026-11-11 10:11

/usr/include/c++/15/string_view:260: ... operator[](size_type) const ...: Assertion '__pos < this->_M_len' failed.
Program received signal SIGABRT, Aborted.

#6  std::basic_string_view<char>::operator[] (__pos=3) at /usr/include/c++/15/string_view:260
#7  do_get (ch=..., args=...) at src/act_obj.cpp:707
#8  loot_and_sacrifice_corpse (looter=..., victim=..., victim_room_vnum=3713) at src/fight.cpp:565
#9  damage (ch=..., victim=..., raw_damage=2, ...) at src/fight.cpp:813
#10 one_hit (...) at src/fight.cpp:535
#11 multi_hit (...) at src/fight.cpp:263
#13 violence_update () at src/fight.cpp:110
#14 update_handler (mud=...) at src/update.cpp:874
#15 MudImpl::game_loop (...) at src/MudImpl.cpp:332

// 'get all' or 'get all.obj' (in the room)
const auto is_all = matches(arg1, "all");
const auto named = is_all ? std::string_view{} : arg1.substr(4);  // after "all."
found = false;
for (auto *obj : ch->in_room->contents) {
    if ((is_all || is_name(named, obj->name)) && ch->can_see(*obj)) {
        found = true;
        get_obj(ch, obj, nullptr);
    }
}
if (!found) {
    if (is_all)
        ch->send_line("I see nothing here.");
    else
        act("I see no $T here.", ch, nullptr, named, To::Char);
}

bool is_mass_looting_npc_undroppable_obj(const Object *obj, const Object *container, const bool unnamed_all) {
    return container->type == ObjectType::Npccorpse && unnamed_all && (obj->is_no_drop() || obj->is_no_remove());
}
// line 707:
if (is_mass_looting_npc_undroppable_obj(obj, container, is_all)) {

Focus

• loot_and_sacrifice_corpse (src/fight.cpp:559, 22 lines): src/fight.cpp:565 is in it
• do_get (src/act_obj.cpp:613, 115 lines): src/act_obj.cpp:648 is in it
• is_mass_looting_npc_undroppable_obj (src/act_obj.cpp:426, 4 lines): named in the idea
• Commands: get, take

1. Explorer insights

• Bug check code.get-all-past-end (fixed, crash): get all and autoloot read past the end of "all". mentions do_get, is_mass_looting_npc_undroppable_obj
• Bug check code.vampiric-integer-drain (present, wrong-behaviour): Vampiric weapons drain nothing from hits under 100 damage. fight.cpp:718, fight.cpp:719

To decide

• The fix should turn the code check above to fixed (it matches the faulty pattern). Does it, and are there other occurrences to fix at the same time?

2. Architecture

• Reported call path (innermost first): do_get (src/act_obj.cpp:707) <- loot_and_sacrifice_corpse (src/fight.cpp:565) <- damage (src/fight.cpp:813) <- one_hit (src/fight.cpp:535) <- multi_hit (src/fight.cpp:263) <- violence_update (src/fight.cpp:110) <- update_handler (src/update.cpp:874) <- MudImpl::game_loop (src/MudImpl.cpp:332)
• loot_and_sacrifice_corpse is defined at src/fight.cpp:559-580 (22 lines): void loot_and_sacrifice_corpse(Char *looter, Char *victim, sh_int victim_room_vnum)
• Called from damage (src/fight.cpp:586)
• loot_and_sacrifice_corpse calls ArgParser (3 definitions), check_enum_bit, do_get, do_sacrifice, empty (3 definitions), get_obj_list, is_npc, is_pc
• do_get is defined at src/act_obj.cpp:613-727 (115 lines): void do_get(Char *ch, ArgParser args)
• do_get handles the command(s) get (minimum position Resting, src/InterpreterImpl.cpp:104), take (minimum position Resting, src/InterpreterImpl.cpp:238)
• Called from loot_and_sacrifice_corpse (src/fight.cpp:559); so it also runs without a player typing it
• do_get calls act (7 definitions), can_loot, can_see (4 definitions), check_enum_bit, empty (3 definitions), get_obj, get_obj_here, get_obj_list, is_mass_looting_npc_undroppable_obj, is_mortal, is_name, matches (2 definitions) and 3 more
• is_mass_looting_npc_undroppable_obj is defined at src/act_obj.cpp:426-429 (4 lines): bool is_mass_looting_npc_undroppable_obj(const Object *obj, const Object *container, const char looting_all_item_dot)
• Called from do_get (src/act_obj.cpp:613)
• is_mass_looting_npc_undroppable_obj calls is_no_drop, is_no_remove (2 definitions)
• src/fight.cpp: 2498 lines, 47 functions
• src/act_obj.cpp: 2199 lines, 43 functions

To decide

• Is the change local to these functions, or does the fix belong in a shared helper their callers use too?
• Does every caller (including automatic ones) still get the behaviour it expects?

3. Design patterns

• In loot_and_sacrifice_corpse: ArgParser x3 (modern)
• In do_get: ArgParser x1 (modern), matches()/matches_start() x7 (modern), range-for over contents x2 (modern), act() x9 (either), '\0' terminator checks x2 (legacy)
• do_get mixes modern (ArgParser, matches()/matches_start(), range-for over contents) and legacy ('\0' terminator checks) idioms: the modern ones already in the function are the pattern to follow.
• do_get already names a test as a value: is_all = matches(arg1, "all")
• do_get already names a test as a value: is_all_named = matches_start("all.", arg1)
• 140 other functions share the signature (Char *ch, ArgParser args); in the same file: do_buy, do_donate, do_drink, do_drop, do_eat, do_fill, do_give, do_list and 15 more
• In is_mass_looting_npc_undroppable_obj: '\0' terminator checks x1 (legacy)
• Across src/: act() 779, std::string_view 625, ArgParser 402, matches()/matches_start() 291, range-for over contents 267, fmt::format 211, sprintf/strcpy/strcat 19, '\0' terminator checks 14, char buffers 10

To decide

• Which existing idiom does the change follow, and does it leave the function more consistent?
• Is there a helper already (in the same file or a header) that does what the change needs?

4. Technical debt

• do_get indexes string_views like C strings (reading past the end when the view is that long, and never seeing a terminator): arg1[3] at src/act_obj.cpp:641, &arg1[4] at src/act_obj.cpp:641, arg1[3] at src/act_obj.cpp:648, &arg1[4] at src/act_obj.cpp:651, arg1[3] at src/act_obj.cpp:707
• do_get is 115 lines long (src/ median 7).
• Legacy idioms in do_get: '\0' terminator checks
• Legacy idioms in is_mass_looting_npc_undroppable_obj: '\0' terminator checks
• The same pattern elsewhere in src/ (11 places in 4 functions; each is the same kind of bug unless the view is known to be long enough): do_who arg[0] :1249; do_permit player[0] :95, player[0] :98; skill_lookup name[0] :39, name[0] :39; nanny argument[0] :711, argument[0] :740, argument[0] :762, argument[0] :854, argument[0] :890, argument[0] :915

To decide

• Are the other places with the same pattern the same bug, and do they belong in this change?
• What debt does the change pay down while it's here, and what does it deliberately leave?
• Does the change add any (a special case, a duplicated check, a new legacy idiom)?

5. Gameplay

• Players reach this with get (from position Resting up).
• Players reach this with take (from position Resting up).
• Messages loot_and_sacrifice_corpse can show: "all corpse"; "gold corpse"
• do_get also runs on its own from loot_and_sacrifice_corpse (src/fight.cpp:559): players who never type the command are affected too (e.g. through an auto setting).
• Messages do_get can show: "Get what?"; "I see no $T here."; "I see nothing here."; "I see no $T here."; "You can't do that."; "I see no $T here."; "That's not a container."; "You can't do that."; "The $d is closed."; "I see nothing like that in the $T." (+6)
• Balance numbers Explorer models from these files: none (the change is behaviour, not balance; Explorer's predictions shouldn't move)

To decide

• What does a player notice before and after (messages, what they get, what's refused)?
• Can a careless or abusive player use the change (or the old behaviour) to their advantage?
• Does it change balance for any class, race or level range? If so, which Explorer numbers?

6. Documentation

• Help DROP GET GIVE PUT TAKE (area/Help.are:467, level 0): Syntax: drop <object>

To decide

• Does any help entry describe the behaviour being changed, and does it need rewording?
• Does the change need a note anywhere else (README, release notes, an issue comment)?

7. Test coverage

• No unit test file for src/fight.cpp.
• src/test/act_obj_test.cpp: named after act_obj.cpp
• No unit test calls loot_and_sacrifice_corpse.
• No unit test calls do_get.
• No unit test calls is_mass_looting_npc_undroppable_obj.
• Sim scenarios/test_live_bugs.py sends get
• Sim xania_sim/activities/loot.py sends get
• Sim finding crash:SIGABRT:act_obj.cpp:do_get [known, seen 5x]: xania crashed (SIGABRT, failed assertion) in do_get (act_obj.cpp) (https://github.com/abraidotti/xania/issues/1)

To decide

• Which unit test pins the fixed behaviour (a test that fails before the change)?
• Which Sim trial or scenario shows it from a player's side, and should a new one be added?

8. History

• arg1 is declared at src/act_obj.cpp:617, a line last changed in 35c8f3a (2021-12-03, snellers): get, put, freeze & gain commands upgraded to ArgParser or string_view....
• arg1[3] at src/act_obj.cpp:641 was last changed in 5939d45 (2022-07-04, snellers): Remove can_see_obj().
• &arg1[4] at src/act_obj.cpp:641 was last changed in 5939d45 (2022-07-04, snellers): Remove can_see_obj().
• arg1[3] at src/act_obj.cpp:648 was last changed in 6f770fb (2021-06-20, snellers): merc.h & xania.cpp cleanup: moving check_material_vulernability(), can_loot(), obj_move_violates_uniqueness() into act_obj.[ch]pp.
• &arg1[4] at src/act_obj.cpp:651 was last changed in 5216c9f (2020-08-12, Matt Godbolt): act() refactor: introduce a strong type for "To"
• arg1[3] at src/act_obj.cpp:707 was last changed in 5e72201 (2021-04-16, snellers): 'get all corpse' including the autoloot feature no longer auto loots NODROP or NOREMOVE items (NPC corpses only). Instead the looter and other chars in the room are warned. This is to prevent accidental looting of nodrop/noremove items.
• The conversion to string_view (or ArgParser): 35c8f3a (2021-12-03, snellers): get, put, freeze & gain commands upgraded to ArgParser or string_view..... The indexing above has been on a view, with no terminator, since then
• loot_and_sacrifice_corpse (22 lines) comes from 8 commits; the most: 5db75c2 (2020-07-30, snellers): If a pet gets the killing blow, its owner is chosen as the autolooter. Resolves #60 (9 lines); 39ee043 (2021-10-25, snellers): Macro death: convert PLR_ act flags to PlayerActFlag. (6 lines); 35c8f3a (2021-12-03, snellers): get, put, freeze & gain commands upgraded to ArgParser or string_view.... (2 lines); b1c5588 (2020-08-29, Matt Godbolt): CHAR_DATA -> Char (1 lines)
• do_get (115 lines) comes from 16 commits; the most: 33a9553 (2020-07-05, Matt Godbolt): Initial import of just code, areas, and html (41 lines); 35c8f3a (2021-12-03, snellers): get, put, freeze & gain commands upgraded to ArgParser or string_view.... (17 lines); 6f770fb (2021-06-20, snellers): merc.h & xania.cpp cleanup: moving check_material_vulernability(), can_loot(), obj_move_violates_uniqueness() into act_obj.[ch]pp. (13 lines); 5216c9f (2020-08-12, Matt Godbolt): act() refactor: introduce a strong type for "To" (7 lines)
• is_mass_looting_npc_undroppable_obj (4 lines) comes from 4 commits; the most: e0683fb (2021-07-03, snellers): merc.h cleanup: move OBJ_DATA to Object.hpp and rename to Object. (1 lines); 49cb513 (2021-07-05, snellers): merc.h cleanup: ITEM_ type defines converted to ObjectType enum.... (1 lines); 88ff199 (2021-07-08, snellers): merc.h cleanup: replace IS_OBJ_STAT macro with object extra checker methods. (1 lines); 33a9553 (2020-07-05, Matt Godbolt): Initial import of just code, areas, and html (1 lines)
• Since 35c8f3a (2021-12-03), 26 commits changed the compiler or its flags; the latest: 7bd8c82 (2026-10-31, snellers): github action upgrade to Ubuntu 26.04 and g++-15; 82823ff (2026-10-31, snellers): General tooling upgrades, largely for Ubuntu 26.04.1 compatibility conda 26.9.4-0 conan 2.33.0 cmake >= 3.31 g++-15 default, g++-13 fallback Compiles on ubunt 26.04.1; 425fd97 (2025-04-07, snellers): Docker: ubuntu 24 is the default these days. Install clang 19. Stop trying to install awscli as it's not in the ubuntu apt sources now. The session doesn't start tmux by default. [skip ci]; e5483b6 (2025-03-28, snellers): Update g++ versions in README. Retire the g++ 10/11 toolchains.; 55c9b1b (2025-03-28, snellers): github: upgrade to ubuntu 24 and use g++-13
• Debug builds add -fno-omit-frame-pointer -fsanitize=leak -fsanitize=address -fsanitize=undefined
• Release builds add -O2 -DNDEBUG -Wno-error=array-bounds
• Upstream issue #72 (closed): The autoloot routine loot_and_sacrifice_corpse() has a chance it might target the wrong corpse. (mentions loot_and_sacrifice_corpse)

To decide

• When did the behaviour start, and why did nobody notice until now?
• Did a toolchain change (compiler, flags, sanitizers) make it show, or change how it shows?
• Has upstream seen or tried this before, and does the change fit what was decided then?

Gate plan

What prototype runs on the fork before this can ship:

• Builds: Debug (ASan, UBSan, LSan, libstdc++ assertions), Release (-O2)
• Unit tests: src/test/act_obj_test.cpp
• Sim: smoke: the game still boots and a player can log in
• Sim: hunt --minutes 5: fights and what runs after a kill (autoloot) reach it
• Sim: pytest scenarios/test_live_bugs.py: it sends the commands involved
• Regression: Sim finding crash:SIGABRT:act_obj.cpp:do_get should stop reproducing (https://github.com/abraidotti/xania/issues/1)