Off-by-one range checks in the client (one crashes at startup)

[mcfnord]

🤖 AI: Three input-range checks in the client are off by one or missing. One reads past the end of a vector and crashes the client at startup; the other two are non-crashing JSON-RPC contract defects in the same family. All measured on main @ 8b667a3a, x86-64.

1. customdirectoryindex — out-of-bounds read, SIGSEGV at startup

The client reads customdirectoryindex from the ini with an inclusive upper bound of MAX_NUM_SERVER_ADDR_ITEMS, then uses it as a subscript into a vector of exactly that many elements, so the accepted value one past the end crashes the client at startup.

src/settings.cpp:786 accepts 0..MAX_NUM_SERVER_ADDR_ITEMS, and GetNumericIniSet's upper bound is inclusive (src/settings.cpp:144). MAX_NUM_SERVER_ADDR_ITEMS is 12 (src/global.h:218) and vstrDirectoryAddress holds 12 elements (src/settings.h:178), so 12 is accepted and 12 is out of range. Two call sites subscript with it unguarded: src/connectdlg.cpp:320 and src/clientrpc.cpp:211.

Both keys are needed to reach it: the index is only read when directorytype is 7 (AT_CUSTOM), and winviscon=1 opens the connect dialog at startup. Measured with QT_QPA_PLATFORM=offscreen, Qt 5.15.3.

customdirectoryindex	result	connect dialog drawn
11	runs normally	yes
12	SIGSEGV, exit 139	no

Under valgrind the 12 arm reports, and the 11 arm reports no invalid access at all:

Invalid read of size 8
   at NetworkUtil::GetDirectoryAddress(EDirectoryType, QString const&)
   by CConnectDlg::RequestServerList()
   by QWidget::event(QEvent*)
Invalid read of size 4
   (same two frames)

The client's own writer only ever emits 0..11 (src/settings.cpp:1006), so reaching this needs a hand-edited or corrupted ini rather than ordinary use. The range check exists to make that safe, and it is off by one.

Fix: GetNumericIniSet ( ..., 0, MAX_NUM_SERVER_ADDR_ITEMS - 1, iValue ). Rebuilt with only that change, same rig, both arms:

customdirectoryindex	unpatched	with - 1
11	runs, dialog drawn	runs, dialog drawn
12	SIGSEGV	runs, dialog drawn

2. jamulusclient/setFaderLevel — accepts one channel past the end

Valid fader indices are 0..MAX_NUM_CHANNELS-1 = 0..149 (vecpChanFader is sized MAX_NUM_CHANNELS at src/audiomixerboard.cpp:1020; MAX_NUM_CHANNELS is 150 at src/global.h:233). The validator rejects only channelIndex > MAX_NUM_CHANNELS (src/clientrpc.cpp:371), so 150 is accepted. Driven over the JSON-RPC socket:

channelIndex	response
149	{"result":"ok"}
150	{"result":"ok"}
151	{"error":{"code":-32602,"message":"...out-of-range"}}

Unlike case 1 this cannot crash: every consumer re-guards with < MAX_NUM_CHANNELS (src/audiomixerboard.cpp:1461, and the headless path at src/client.cpp:954), so channel 150 is silently dropped while the caller is told "ok". Fix: > → >=. The house pattern elsewhere is exclusive already — MathUtils::InRange<int> ( id, 0, iMaxNumChannels ) at src/server.cpp:1386.

3. jamulusclient/setSkillLevel with null — response carries neither result nor error

null is a documented-valid value that clears the skill level. Its branch (src/clientrpc.cpp:304) sets SL_NOT_SET, calls SetRemoteInfo(), and returns at src/clientrpc.cpp:308 without assigning response["result"]:

skillLevel	response
null	{"id":4,"jsonrpc":"2.0"}
"expert"	{"result":"ok"}

A response with neither member violates JSON-RPC 2.0 §5, and the method's own doc says @result — Always "ok". A strict client waiting for result blocks on a valid call. Fix: add response["result"] = "ok"; before the return.

---

🤖 This message was written by AI and reviewed by @mcfnord.