Coverity Backlog Audit - 2026-05-06 (Two-Session Summary) =========================================================

This is a recap of a systematic audit through the Coverity backlog as
reported in this msgbase. Goal was to clear the backlog rather than
continue handling defects ad-hoc as new ones land.

Scope
-----

Source: /home/rswindell/sbbs (active master).

Excluded from scope (by request, since they have their own owners or
defect characteristics):
* src/conio/* - terminal-abstraction layer
* src/syncterm/* - separate program/repo
* src/ssh/*
* src/sftp/* - the standalone SFTP library (sbbs3/sftp.cpp,
the SBBS-side server glue, stayed in scope)
* 3rdp/* - vendored upstream code; flagged for "Intentional"
marking via the Coverity Scan UI rather than
local patches

After exclusions: 163 unique unresolved CIDs in scope across the
1-CRITICAL, 2-HIGH, and 3-MEDIUM tiers.

Results
-------

46 commits citing 60 CIDs were merged across the two sessions, plus
five more CIDs (469134, 470386, 470388, 470390, 479098) that should
clear automatically once Coverity propagates the js_do_lock_input
root SUPPRESS at js_console.cpp.

Roughly 38 additional CIDs were discovered to already be mitigated
during verification - the original defective code line was still
matchable by grep, but surrounding guards/clamps/refactors made the
defect benign. About half of the supposedly-unresolved CIDs in any
given batch turned out to be in this state. (Lesson: never trust a STILL_PRESENT bucket built only from grep on the flagged code line;
always read the surrounding ~20 lines first.)

Closed code work, by tier:

Tier 1 (CRITICAL, 27 in-scope) - closed
14 fixed by code, 10 already-mitigated, 3 vendored (3rdp/).

Tier 2 (HIGH, 21 in-scope) - 16 closed; 5 deferred for policy
decisions (see below).

Tier 3 (MEDIUM, 125 in-scope) - concurrency cluster fully closed;
long-tail RESOURCE_LEAK / INTEGER_OVERFLOW / CHECKED_RETURN
batches closed; architectural clusters (Y2K38, JS_ValueTo*)
explicitly deferred.

Notable real bug fixes (not just SUPPRESS comments) ---------------------------------------------------

* js_socket.cpp js_connected_socket_constructor (CID 530501) -
p->sock was assigned to a real socket then the function could
goto fail and free p without closesocket(). Now p->sock is set
to INVALID_SOCKET right after malloc/memset, and the fail label
closesocket()s when valid.

* smbutil maint() (CID 644892) - five "if (terminated) return;"
sites were leaking idxbuf, the SMB header lock, and (in the
deletion-execution loop) the SMB-allocation file handles.

* smbutil packmsgs() (CID 462184) - three bare returns on fread
failure leaked datoffset.

* websrvr getuserdat error handling (CIDs 516407, 516410, 639949) -
the user struct was being read into a partially-populated state
on failure and then used downstream for password compare or
session state.

* useredit user_config() (CIDs 516411, 530902) - silently
swallowed getuserdat failures after the JS user-config module
ran. Now logs through errormsg(WHERE, "reading", ...) like
purgeuser().

* xpbeep reaper (CIDs 645736, 645739) - took r->mutex briefly
before reading r->auto_close/r->done; the previous code read
them while another thread could be mutating, and the reaper
then destroys the mutex.

* mailsrvr sockmimetext (CID 639931) - bounded the per-line scan
with strnlen to stop Coverity from seeing an unbounded read
past the buffer.

* js_socket js_sendto getaddrinfo (CID 639937) - paren bug:
'if ((result = getaddrinfo(...) != 0))' parses as
'result = (getaddrinfo(...) != 0)' so the result is 0/1 instead
of the EAI_* error code.

* smbutil packmsgs() / writemsg movemsg / dupefind - bundled
fseek with the subsequent fread error path so a seek failure
no longer lands on the wrong offset silently.

* useredit, ssbsecho, sftp, mqtt, websrvr, ftpsrvr - 17 CHECKED_RETURN
sites where the discarded return is genuinely best-effort
(chmod/remove/cryptSetAttribute/setsockopt/strlcat etc.) now
have explicit (void) casts so the intent is documented.

False positives suppressed with explanation -------------------------------------------

The link_list_t recursive mutex (link_list.h:99) is the source of
many Coverity LOCK false-positives, because Coverity does not track
that the helper functions that call listLock/listUnlock internally
form a balanced lock-unlock pair on each call. SUPPRESS comments
with explanation were added at:

* userdat login* family (631133, 631140, 631141, 631146)
* services login_attempt_list (631138, 631139, 639948, 643135)
* mailsrvr/sbbscon (631134, 631143, 631144)
* websrvr send_error ORDER_REVERSAL (631137)

Other suppressions:

* ssl.c destroy_session (479100, 530506) - sess_list and cert_list
are separate lists with separate mutexes; the node is exclusively
owned by the current thread between removal-from-sess_list and
append-to-cert_list.
* js_console js_do_lock_input (469125 + caller chain) - asymmetric
contract; documented at the function head. Caller chain (469134,
470386, 470388, 470390, 479098) should clear on Coverity rescan
once the root suppress propagates.
* main.cpp output_thread GCESSTR-while-ssh_mutex (469167) - the
intentional design; releasing the mutex across the error report
would race state.
* main.cpp/logfile.cpp nodefile_mutex inter-procedural (515594-6,
543172) - mutex confined to getnodedat/putnodedat by design.

Deferred items (need policy decisions)
--------------------------------------

5 Tier 2 CIDs:
470556 mailsrvr.cpp pop3 APOP - DC.WEAK_CRYPTO rand() for nonce.
Real concern in the MD5 challenge-response, but fix needs
a project-level CSPRNG choice (no xp_secure_random exists).
643145 ftpsrvr.cpp tmpfname - DC.WEAK_CRYPTO rand() for filename
uniqueness. Not security-critical; suggest "Intentional"
via Coverity Scan UI.
487170 sftp.cpp sftp_open - TOCTOU access() then create. Benign
in the SFTP-server context; suggest "Intentional".
644193 websrvr.cpp - REVERSE_NEGATIVE on session->socket. Likely
a false positive (the socket is checked at the boundaries
Coverity examines).
639939 websrvr.cpp - same as 644193, different site.
645800 sftp.cpp:2184 - TAINTED_SCALAR malloc(path->len + 1).
Easy to add an upper bound but wanted input on the bound.

JS_ValueToInt32 / JS_ValueToECMAUint32 cluster (8 in-scope CIDs):
470929, 532317, 639930, 639933, 639942, 639945, 639946, 640403.
All 8 have callers that pre-initialize the int32 result to a safe
default, so the unchecked-return failure is benign. There are 17+
additional unchecked sites in js_system.cpp alone that Coverity
has not flagged. Mechanical (void) casting on only the flagged
sites would be inconsistent. Suggest a project-level decision:
either rewrite all callers strictly, or mark these as Intentional
in the Coverity Scan UI.

Y2K38_SAFETY cluster (12 CIDs across mailsrvr/ftpsrvr/services/
websrvr *_server() functions, sexyz send_files/receive_files,
sftp.cpp dirent attrs, getstats.c, etc.) - most are deliberate
(time32_t)t casts in wire/persistent formats. Real fix requires
changing struct layouts and breaking on-disk/wire compatibility.
Architectural decision, not per-CID.

3rdp/ vendored cluster (5 CIDs in mozjs libffi, mozjs build
artifact, cryptlib dn_string, ext_copy.c) - request "Intentional"
or "False Positive" classification via the Coverity Scan UI.

filterfile.hpp 643146 - deliberate sleep-while-locked under
lock_guard during file refresh. Defensible design; suggest
"Intentional" via the Coverity Scan UI.

Methodology lessons
-------------------

* Verify before fix. About half of the supposedly-unresolved
CIDs in each batch had been silently mitigated by surrounding
code changes that didn't reference the CID number in the
commit message. Reading 20 lines of context around the flagged
line beats trusting STILL_PRESENT grep.

* "git log --grep=CID NNNNNN" finds explicit fixes quickly. But
range-format commit messages like (CIDs 631068-631076) won't
expand for the inner CIDs - either expand ranges before greping
or fall back to git log -S on the flagged code line.

* Atomic-conversion silently fixes related Coverity CIDs. When a
field becomes std::atomic<T> (e.g. sys_status, ssh_mode,
telnet_mode), all LOCK_EVASION / MISSING_LOCK / ATOMICITY CIDs
touching that field auto-resolve. Always grep sbbs.h for
std::atomic declarations of the flagged field before assuming
a race remains.

* link_list_t recursive-mutex helpers (listLock/listUnlock) are
a major source of Coverity LOCK / ORDER_REVERSAL / SLEEP
false-positives. Coverity does not track inter-procedural
helpers, so adjacent suppress comments are the right answer
when the helper internally forms a balanced lock-unlock pair.

* For SUPPRESS comments, prefer explaining the invariant rather
than just naming the checker. Future readers (and the next
Coverity rescan reviewer) will understand the decision.

Next steps
----------

After the next overnight Coverity scan, the SUPPRESS-propagation
items (469134, 470386, 470388, 470390, 479098) should clear. The
deferred clusters above are awaiting policy decisions; happy to
implement either direction once decided. Any new digest will
benefit from the cleaner baseline - only genuinely-new defects
should appear.

Open to discussion on the deferred items.


---
* Synchronet * Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net