Guard GetValFromStream against out-of-bounds reads in release builds

[mcfnord]

Fixes #3819

GetValFromStream validates its arguments only with Q_ASSERT, which is compiled out in release builds — the configuration everyone actually ships. CVector derives from std::vector and uses its unchecked operator[], so a truncated or malformed network message can drive vecIn[iPos] past the end of the buffer with no protection at all in release.

Approach

Keep the asserts (developer errors still fail loudly in debug builds) and add a runtime guard that returns 0 when iNumOfBytes is outside 1..4 or when fewer than iNumOfBytes bytes remain:

Q_ASSERT ( ( iNumOfBytes > 0 ) && ( iNumOfBytes <= 4 ) );
Q_ASSERT ( vecIn.Size() >= iPos + iNumOfBytes );

if ( ( iNumOfBytes < 1 ) || ( iNumOfBytes > 4 ) || ( vecIn.Size() < iPos + iNumOfBytes ) )
{
    return 0;
}

Returning 0 (rather than crashing) keeps the existing "malformed input is tolerated, not fatal" posture of the surrounding parser — the sibling GetStringFromStream already returns an error code on short input rather than aborting. This is the minimal, hot-path-safe change and touches no call sites.

Open question for maintainers, per the issue: if a hard failure / disconnect on malformed input is preferred over silently returning 0, that would be a larger change (the function has no error channel today, so it would need a signature change across ~50 call sites). Happy to go that direction if you'd rather.

Testing

• clang-format-14 clean
• Headless build + server smoke test (clean startup, clean SIGTERM)

[ann0see]

Might be removed...

[ann0see]

Here's the question what it should actually return...

[softins]

This is conflating an error indication with a fetched value. In my view, the correct fix would be for the GetValFromStream() and GetCountryFromStream() functions to follow the format of GetStringFromStream(), where the output variable is passed by reference as a parameter, and the return value is a bool indicating error status (true) or success (false). Every fetch would then be validated and could abort the Evaluate function as soon as a malformation, bounds overflow or bad value was detected.

This would require all calls within protocol.cpp to be reworked, but I think it's worth doing, and shouldn't take too long. I'm happy to take that on if agreed,

Also, the runtime checks on iNumBytes are only checking against programmer error, not input data error, since every call to GetValFromStream() passes a constant 1, 2 or 4 as iNumBytes.

[ann0see]

The data from the wire is definitely unchecked at the moment then?

[softins]

It looks like some values are checked, and others are just used without checking.

[ann0see]

It's probably worth understanding where it is called and what would happen if 0 is returned.

[mcfnord]

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

Good questions — here's the call-site trace behind returning 0.

Where it's called. GetValFromStream has ~60 call sites, all inside Evaluate* / Parse* message handlers. Every one of them is reached only after ParseMessageFrame has (a) checked the frame's declared length against the datagram size (iLenBy != iNumBytesIn - MESS_LEN_WITHOUT_DATA_BYTE → error) and (b) verified the CRC. So on well-formed traffic iPos + iNumOfBytes ≤ Size() always holds and the guard never fires — zero behavioral change for real clients.

The call sites come in two shapes:

1. List loops — EvaluateCLConnClientsListMes, the server-list parsers, etc. (while ( iPos < iDataLen ) at lines 1192 / 2095 / 2203 / 2446). Each already does its own ( iDataLen - iPos ) < N → return true bounds check at the top of every iteration, so it never under-reads. The new guard is redundant here and, importantly, cannot fire inside these loops, so it can't affect loop termination.

2. Fixed-layout readers — the straight-line handlers that read a known field sequence, trusting the frame length/CRC check above them. These are the ones a truncated-but-CRC-valid or version-mismatched body could push past the end. This is the case the guard exists for.

What happens when 0 is returned. It only happens on already-malformed input. There, 0 flows into a field that is then either range-checked/clamped downstream (e.g. the iChanID bounds check from #3810, or the instrument/skill/country validation) or benignly stored. iPos is left unadvanced, so any following reads in the same fixed message also return 0 — a harmless zero-cascade, not a hang (no caller loops on iPos without its own size guard). In every case 0 is strictly safer than the status quo, which is an unchecked std::vector::operator[] read past the end — undefined behavior, and the direct cause of the #3819 / #3747 out-of-bounds crashes.

On your two inline notes:

• "Might be removed" (the Q_ASSERTs): I'd keep them. They catch a developer mistake — a caller passing iNumOfBytes outside 1..4 — loudly in debug builds. The runtime guard covers a different audience: network malformation in release builds. Both are cheap; they don't overlap.
• "the question what it should actually return": 0 matches the parser's existing "tolerate malformed input, don't abort" posture — the sibling GetStringFromStream already returns an error code on short input rather than crashing, and every caller here already treats these values as untrusted. The alternative — a hard disconnect on malformed input — needs an error channel the function doesn't have today, which is a signature change across ~50 call sites. Happy to go that way if you'd prefer it, but 0 is the minimal, hot-path-safe fix that closes the crash.

[ann0see]

So on well-formed traffic iPos + iNumOfBytes ≤ Size() always holds and the guard never fires — zero behavioral change for real clients.

If this is true, we could probably get away by just adding a @requires as documentation