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

[mcfnord]

MY LLM WROTE:

On the @requires suggestion — I'd keep the runtime guard rather than replace it with documentation.

The precondition (iPos + iNumOfBytes <= Size()) isn't something the type system or calling convention enforces; it depends on the frame's declared length matching the datagram it actually got, and on the message ID matching the body that was sent for it. A truncated-but-CRC-valid or version-mismatched body violates it — that's the literal mechanism behind #3819/#3747, not a hypothetical. A @requires comment documents that assumption; it doesn't check it, and nothing else in the call path re-verifies it between the frame check and the read.

That leaves this guard covering a different failure than the Q_ASSERTs already in the file. The asserts catch a developer passing a bad iNumOfBytes, and only in debug builds. This guard catches the network sending a bad frame, in the release builds that actually ship. Swapping it for a comment closes the debug-only channel and leaves the release-only one — the one that matters here — covered by nothing.

There's no efficiency case for the swap either: the call-site trace above already shows the guard costs nothing on well-formed traffic (never taken) and can't affect the list-loop callers (each already bounds-checks per iteration). It only does anything on a malformed body — where the alternative is the unchecked operator[] read this PR exists to fix.

[mcfnord]

MY LLM WROTE:

@softins You're right on the iNumBytes point, and I checked rather than assuming: of the 63 GetValFromStream call sites in protocol.cpp, 62 pass a literal 1, 2 or 4 (33/18/11). The 63rd is inside GetStringFromStream and passes iNumberOfBytsLen, which is itself only ever the default 2 or the literal 1 at line 2270. So every reachable value is a compile-time constant, and both the assert and my guard's iNumOfBytes half are pure programmer-error checks that no wire data can trip. That half earns nothing.

The half that does the work is vecIn.Size() < iPos + iNumOfBytes — that one depends on the frame contents, and it's the one standing between a malformed body and the unchecked std::vector::operator[].

On the larger refactor: I agree it's the correct end state, and I'd rather have it than what I wrote. My earlier comment defended returning 0 as a "harmless zero-cascade," and that's accurate about safety but it concedes the actual point — a handler that keeps parsing a body it already knows is malformed, storing zeros as it goes, is doing work it shouldn't be doing. Detecting at the read and aborting the Evaluate* is a real improvement, not just a stylistic one.

One argument for your design that hasn't been raised: GetCountryFromStream (line 2889) structurally cannot report an error any other way. It returns a QLocale::Country, and there's no value in that enum that means "malformed" as opposed to a country — it just forwards whatever 2 bytes it read into WireFormatCountryCodeToQtCountry. A sentinel return can't work there, so the out-param + bool form isn't merely nicer for it, it's the only shape that can carry the error at all. That makes the two functions consistent for a reason rather than by preference.

So — @ann0see, this wants a decision more than it wants more analysis from me. Two coherent options:

1. Merge Guard GetValFromStream against out-of-bounds reads in release builds #3821 as an interim guard (it closes the release-build UB in ~5 lines and changes nothing on well-formed traffic), and let the refactor land separately on top, deleting the guard when it does.
2. Close Guard GetValFromStream against out-of-bounds reads in release builds #3821 and go straight to softins's version.

I have no stake in which. softins offered to take the refactor on, and if that's the plan I'm happy to close this PR now so it isn't sitting in the queue implying a decision that's already been made — just say the word. The only outcome I'd argue against is the thread staying open with neither happening, since the unchecked read is live in shipped release builds today either way.