DeferTask/DeferImmediateTask return bool (nothrow OOM); wait_que and
sql_que free the BQUE and refund quota/waitcost when enqueue fails.
db_write_object reports stream status via ferror; db_write returns -1
on I/O failure after header/object/end/flush checks, and dump/dbconvert
callers refuse to publish a truncated flatfile.
Replace ~900 typographic \xE2\x80\x.. escapes (curly quotes, en dashes)
and a few \xE2\x80\230 octal workarounds with real UTF-8 in message
strings under mux/modules and mux/src. Leave stringutil and convert
charset mapping tables as explicit byte sequences.
Completes the sweep the issue called for. mux_atol returns long, which
is 32-bit on LLP64, so every caller silently truncated on Windows. Two
of those were real defects (the truthiness family and cf_size, fixed in
the preceding commits); the rest were latent, waiting for a value large
enough to matter.
Rather than audit 290 sites for whether each can reach 2^31 today, use
the 64-bit parser everywhere and remove the class. A dbref cannot
overflow now, but nothing stops a later caller passing that same site a
timestamp or a byte count.
Pure 1:1 substitution: 285 lines changed, and every removed line
contained mux_atol while every added line contains mux_atoi64. No
control flow, no types, no behaviour beyond the wider parse.
This is a NO-OP on LP64 -- long is already 64-bit on Linux and macOS, so
the generated code there is unchanged. It only widens the parse on
Windows. Narrowing destinations are unaffected either way: `int x =
mux_atoi64(s)` truncates exactly as `int x = mux_atol(s)` did, on both
models.
Left alone: mux_atol itself in mathutil, its declaration, and three
comments that name it. Callers that genuinely want 32-bit semantics can
still ask for them; none appear to.
Verified on Windows: full solution builds clean with no new warnings,
smoke is 1418 passed / 16 failed / 0 crashes / 306 of 306 dispatched --
identical to before the sweep, with the same 16 build-configuration
failures (exp3 module not loaded, hmac/digest behind UNIX_DIGEST).
Spot checks after the change: the boolean family returns 1 for multiples
of 2^32, cf_size round-trips 3000000000 and still reads -1 as unlimited,
and arithmetic, string and list functions are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audit of the DB load paths (flatfile reader, mail/malias loader, SQLite
attribute bulk-load) under the malicious/corrupt-database threat model --
the same class as #834-843. Five distinct bugs, all in code the June
hardening (#806/#808/#841/#843) did not reach; each is reachable only from
crafted or tampered database content, not from normal gameplay. Verified by
full rebuild + smoke suite (1264/1264, 0 crashes), which exercises flatfile
export/import and SQLite attribute reads end to end.
1. getstring_noalloc: static buffer overflow (lib/dbutil.cpp)
The escaped-string reader mis-accumulated its output-byte count:
`nOutput = pOutput - p` overwrote the count from prior escape emits
instead of adding to it. On the multi-fgets refill path this
under-decrements nBufferLeft, so the `nBufferLeft <= 0` guard never trips
and the next fgets writes past the 2*LBUF_SIZE+20 static buffer.
Legitimate attributes (<= LBUF) never take the refill path, so this only
fires on a crafted quoted string > ~64KB. Fix: accumulate (+=).
2. make_numlist / malias_read: stack buffer overflow (modules/engine/mail.cpp)
malias_read read the recipient count (numrecep) straight from the file
and pushed that many dbrefs into m->list with no cap, making
m->list.size() attacker-controlled. make_numlist then copied all of
m->list into the fixed stack array aRecip[(LBUF_SIZE+1)/2] with no bound
on nRecip -- a crafted mail.db with numrecep > 16384 overflows the stack
(with attacker-chosen dbrefs) the next time any player mails the alias.
Fix: clamp numrecep to (LBUF_SIZE+1)/2 at load (also bounds the reserve()
that could otherwise exhaust memory on an INT_MAX count), and defensively
bound the copy loop in make_numlist.
3. SQLite bulk-load attribute value: heap buffer overflow (modules/engine/sqlitedb.cpp)
GetAllAttributes/GetBuiltinAttributes passed the raw column blob length to
the cache with no clamp, unlike the write path (cache_put) and the
standalone read path (GetAttribute), both of which clamp to LBUF_SIZE. A
value blob written directly into the SQLite file therefore flows unclamped
to atr_get_str_LEN's `memcpy(s, buff, (*pLen)+1)` into a fixed LBUF_SIZE
buffer, overflowing the heap on first read of the attribute
(Name/look/examine/get). Fix: clamp len to LBUF_SIZE in both bulk-load
functions, mirroring the existing clamps.
4. get_list: infinite loop + unbounded log on truncated flatfile (modules/engine/db_rw.cpp)
get_list had no EOF case: at end-of-file getc() returns EOF, falls to
default, and calls getstring_noalloc(), which makes no progress at EOF
(ungetc(EOF) is a no-op, fgets returns NULL). The for(;;) then spins
forever, pegging a core and emitting log lines. Trigger: a flatfile whose
last object's attribute list is truncated before its '<' terminator. Fix:
add a case EOF that aborts the load.
5. getboolexp1: BOOLEXP subtree leak on malformed v2 lock (modules/engine/db_rw.cpp)
Three error paths in the v2 lock parser returned TRUE_BOOLEXP without
freeing the partially built node/subtree (bad connective, missing ')',
EOF mid lock-string). Bounded and single-shot (db_read aborts the whole
load on corruption; nesting capped at 1024), but still a leak on crafted
input. Fix: free the appropriate node/subtree at each site, matching the
partial-construction state.
Also traced and dropped a sixth candidate (SQLite attribute owner read
unvalidated): the attribute owner is never used to index db[] -- only
compared, read for its flags, or re-stored -- so a corrupt owner is harmless.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Loading a corrupt/malicious DB with an out-of-range user-attribute number
corrupted memory or OOMed: the number indexes anum_table via anum_set (a bare
`anum_table[x]=v` macro) and anum_extend (allocates a dense (x+1) table), with
no validation. The read path atr_num validates anum<0||>top; the write path
(vattr_define_LEN -> anum_extend/anum_set) did not (incomplete hardening,
family of #805/#806/#807).
Live-verified via flatfile import (+A record, novel attr name):
- +A-10000000 -> anum_table[-10000000] write -> SIGSEGV.
- +A-5 -> anum_table[-5] write in valid heap -> silent corruption.
- +A999999999 -> anum_extend allocs ~8GB dense table -> OOM kill.
File-derived numbers reach this from the flatfile +A handler (getref) and the
SQLite attr-name load (db.cpp:3626). A huge number also seeds attr_next enormous
via g_max_nam_atr/max_attrnum_loaded -> later runtime OOM.
Fix:
- New A_USER_MAX (0x01000000 = 16M) constant in attrs.h beside A_USER_START.
- vattr_define_LEN rejects number < A_USER_START || > A_USER_MAX (central backstop).
- The flatfile +A handler and the SQLite attr-name callback validate and skip
the bad record (graceful degradation; flatfile path logs it).
Verified bite-then-not-bite: malformed +A flatfiles SIGSEGV/silent-corrupt/
OOM-kill on the old build, load cleanly (bad record skipped) on the fixed build;
valid DB still imports; smoke 1115/1115.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Importing a v1/v2 flatfile (supported; MIN_SUPPORTED_VERSION=1) with a malformed
or deeply-nested object lock crashed during db_read: getboolexp1 (the v2 inline
lock parser, reached when read_key is set) recursed with no depth bound, and
every error path (case EOF, bad syntax, the error: label) was mux_assert(0).
Live-verified (bug-catch) via dbconvert -l:
- lock of '(' x500000 -> unbounded recursion -> stack overflow / SIGSEGV.
- truncated lock (EOF mid-expression) -> mux_assert -> SIGABRT.
The runtime @lock parser already caps nesting at lock_nest_lim (default 20);
this import path was unguarded (incomplete hardening, sibling of #806). Fix:
- thread a `depth` arg through getboolexp1 and bound it at
BOOLEXP_LOAD_NEST_MAX (1024 — far above any real lock, far below stack
overflow);
- convert the mux_assert error paths to set s_boolexp_corrupt and unwind, so
getboolexp()/db_read abort the load cleanly (return -1, rolling back the
in-progress SQLite import) instead of crashing.
Verified: deep-lock and truncated-lock v2 flatfiles SIGSEGV/SIGABRT on the old
build, cleanly rejected (exit 1) on the fixed build; smoke 1115/1115. Valid
locks are unaffected (the change is additive; valid locks never hit the depth
cap or the error paths).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Loading a corrupt or malicious flatfile .db (the portable format admins import
via db_load/dbconvert) could corrupt memory or crash: db_read's '!' object
entry read `i = getref(f)` (an unchecked mux_atol result) and went straight to
db_grow(i+1) + s_Name(i,...)/s_Location(i,...), which index db[i] with no bounds
check (SIZE_HACK == 1, so only db[-1] is valid).
Live-verified on the stock netmux.db with one object header changed:
- !-5 (negative) -> OOB write at db[-5] -> SIGSEGV.
- !999999999 (huge) -> db_grow's MEMALLOC fails -> mux_assert abort (SIGABRT).
The SQLite load path (sqlite_load_game, db.cpp:3694/3735) already validates
`i < 0 || i >= top` before db[i] = …; the flatfile path never got the check
(incomplete hardening, same shape as #805). Add the validation: reject
`i < 0 || i > DB_LOAD_MAX_DBREF` (268M, far above any real game) and return -1
so the caller rolls back the in-progress SQLite import instead of crashing.
Verified: both malformed flatfiles now abort the load cleanly (exit 1, no
SIGSEGV/SIGABRT); the valid flatfile still imports; smoke 1115/1115.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Found writing the #786 regression test: an attribute @set no_eval
survived SQLite restarts but came back WITHOUT the flag after a
flatfile round-trip (db_unload -> db_load, the exact path Makesmoke/
Smoke and any flatfile backup uses).
db_write_object() exported attribute values via atr_get_raw(), but
the cache layer strips the \x01owner🎏 prefix on read and hands
back the cleaned text -- so every per-attribute owner override and
every attribute flag (AF_NOEVAL, AF_LOCK, AF_CASE, ...) vanished from
the export. The import side still decodes the prefix fine; nothing
was left to decode. The smoke corpus never noticed because not one
of its ~340 attributes carried a flag or owner override.
Re-encode on export with atr_encode's rule: default owner and zero
flags write the bare value (byte-identical to the old output for the
overwhelmingly common case); anything else writes the classic
\x01owner🎏value form, which db_read already understands.
Verified: @set #1/attr=no_eval round-trips through
dbconvert -u / -l with the flag intact (export shows
"\x011:1048576:add(%0,%1)"); full smoke 1110/0/0 including the new
TC012, which depends on the flag surviving the Makesmoke flatfile
cycle; 3x200 jit_diff clean; tests/libmux 50/0.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second wave: convert manual alloc/free pairs in cque (1), db_rw (4),
conf (5), db (2), engine_com (6), help (7), levels (2), predicates (5),
player (7), funmath (8). Eliminates ~80 explicit free_lbuf calls
including multi-exit error paths in getboolexp1 (5 frees → 0),
get_list, AnnounceConnect/Disconnect, and the eight NOEVAL function
variants (cand/cor/firstof/allof and bool variants).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add LBuf class to alloc.h: an RAII wrapper around alloc_lbuf/free_lbuf
that moves LBUF_SIZE buffers from the stack to the heap pool. Convert
all 108 non-static UTF8 xxx[LBUF_SIZE] stack arrays across 25 source
files. Static BSS buffers (24) are unchanged.
This eliminates LBUF_SIZE from recursive stack frames, making it safe
to increase LBUF_SIZE without risking stack overflow in the evaluation
pipeline.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When migrated output would exceed LBUF_SIZE, return false so the caller
leaves the attribute in its original V4 form. The indexed base color
survives; only the 24-bit refinement is lost. This is safe degradation
versus the previous behavior of storing a truncated value.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The V4 emitter only wrote per-channel deltas for channels that differed
from the indexed palette entry. Omitted channels inherited their values
from the palette base at runtime via UpdateColorState(). The migration
was incorrectly zeroing these channels instead.
Fix: track the last FG/BG BMP palette index as the scanner advances,
and seed R/G/B from palette[] before applying deltas.
Also fix a potential buffer overrun: a 1-delta V4 color (4 bytes)
expands to 8 bytes in V5, so near-LBUF attributes could overflow.
Add output capacity check before each SMP emission.
Tighten the SMP prefix check in mux_color() (stringutil.h and
t5xgame.cpp) to validate the full 4-byte UTF-8 form, preventing
misclassification of malformed data.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the old variable-length per-channel delta encoding (U+F0000-F05FF,
6 blocks of 256, 7-15 bytes per layer) with a fixed-size 2-code-point
encoding (U+F0000-F3FFF, 4 blocks of 4096, always 11 bytes per layer).
CP1 packs R high nibble + G, CP2 packs R low nibble + B. The XTERM
indexed base (3 bytes) is preserved for all 4 output paths.
The DFA shrinks from 37 to 11 states (1536 SMP entries removed from
tr_Color.txt); SMP detection uses a prefix check before the DFA.
Flatfile version bumped to 5 with automatic V4→V5 migration on load.
All 502 smoke tests and 294 standalone color_ops tests pass.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
