[WIP] Use unused bytes in CNetworkTransportProps for EAudioQuality (backwards compatible)

[dingodoppelt]

#3894 surfaced 4 bytes being unused in CNetworkTransportProps, namely iAudioCodingArg.
This uses those bytes to save the selected audio quality in the client. For backwards compatibility we simply leave the old checks in place. This should be improved, hence a work in progress.

CHANGELOG: Use iAudioCodingArg in CNetworkTransportProps for EAudioQuality

Context: Fixes an issue?

fixes #3896

Does this change need documentation? What needs to be documented and how?

No, just a bug fix

Status of this Pull Request

Proof of concept (not to be merged soon);

What is missing until this pull request can be merged?

• dedicated enum containing all possible audio quality settings combinations (or something similiar for the client and server to share)

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[ann0see]

You may break backwards compatibility to old betas IMO. There was no real release and we should try to have a clean state for each release.

[dingodoppelt]

You may break backwards compatibility to old betas IMO. There was no real release and we should try to have a clean state for each release.

Yes, I'm ok with breaking compatibility with beta clients, but this is an opportunity to overhaul the connection process even for non-raw qualities. We would break compatibility to all clients, if we only rely on those 4 bytes.

[ann0see]

Ok. That's fair then.

[dingodoppelt]

At the moment we collect our info from different enums (EAudChanConf, EAudComprType, ENetwFlags, EAudioQuality) that get used (or not) by client and server independently. We could use the free 4 bytes to create a consolidated source of information that client and server can share.

[dingodoppelt]

This makes the server aware of the audio quality set in the client. Before that we had to guess by the packet sizes.

[dingodoppelt]

I left this in for backwards compatibility. We could deprecate this with a TODO: to be removed, same for the transmitting side.

[mcfnord]

🤖 AI: The size test this replaces was doing two jobs, and only one of them is visible: iCeltNumCodedBytes == sizeof(int16_t) * iClientFrameSizeSamples * vecNumAudioChannels is true only when the byte count equals the destination frame exactly, so it was also the bounds check for the memcpy below it. As one arm of an ||, a declared AQ_RAW selects the raw path on its own while the copy length stays iCeltNumCodedBytes — the client's own iBaseNetworkPacketSize, range-checked only to [10, 20000]. The destination is fixed at 512 bytes.

Now it's measured, ASan, server-only build, three UDP datagrams and no credentials:

branch	iAudioCodingArg	declared size	result
f7fbedd9	AQ_RAW	2048 B	heap-buffer-overflow, WRITE 2048 into 512
f7fbedd9	AQ_RAW	20000 B	WRITE of size 20000
f7fbedd9	AQ_LOW	2048 B	clean
main ea14cb94	AQ_RAW	2048 B	clean

Both write sites fire: the memcpy, and the memset on a lost packet. The declared flag is the trigger, not the packet size, and main is unaffected.

A path that keeps the direction: iAudioCodingArg arrives unvalidated — no range check, unlike every field around it, which cost nothing while nothing read it. Validating it on receipt, and keeping the size equality as a requirement rather than an alternative, gives the explicit negotiation without letting the declaration widen a copy. Separately, iAudioCodingArg is initialised in neither the constructor nor ResetNetworkTransportProperties().

The reproducer is a short Python script against a stock build; I can post it here or as a gist.

[dingodoppelt]

🤖 AI: The size test this replaces was doing two jobs, and only one of them is visible: iCeltNumCodedBytes == sizeof(int16_t) * iClientFrameSizeSamples * vecNumAudioChannels is true only when the byte count equals the destination frame exactly, so it was also the bounds check for the memcpy below it. As one arm of an ||, a declared AQ_RAW selects the raw path on its own while the copy length stays iCeltNumCodedBytes — the client's own iBaseNetworkPacketSize, range-checked only to [10, 20000]. The destination is fixed at 512 bytes.

Now it's measured, ASan, server-only build, three UDP datagrams and no credentials:
branch iAudioCodingArg declared size result
f7fbedd9 AQ_RAW 2048 B heap-buffer-overflow, WRITE 2048 into 512
f7fbedd9 AQ_RAW 20000 B WRITE of size 20000
f7fbedd9 AQ_LOW 2048 B clean
main ea14cb94 AQ_RAW 2048 B clean

Both write sites fire: the memcpy, and the memset on a lost packet. The declared flag is the trigger, not the packet size, and main is unaffected.

A path that keeps the direction: iAudioCodingArg arrives unvalidated — no range check, unlike every field around it, which cost nothing while nothing read it. Validating it on receipt, and keeping the size equality as a requirement rather than an alternative, gives the explicit negotiation without letting the declaration widen a copy. Separately, iAudioCodingArg is initialised in neither the constructor nor ResetNetworkTransportProperties().

The reproducer is a short Python script against a stock build; I can post it here or as a gist.

Of course we can break things if we want. The question is (and I'm not asking the AI) whether it works with a client built from main, nothing more. @mcfnord: If you have something to add personally feel free but please don't let the AI post stuff here. It isn't very helpful and its a lot of text I'd rather not have to read.

[mcfnord]

but please don't let the AI post stuff here. It isn't very helpful and its a lot of text I'd rather not have to read.

I have tried to slash the word count, but not sure that has made things easier to read. I generally respect the policy each person sets. When you say "I doubt that's true" my AI will respond with more evidence rather than less, so just be aware of that. When you say "here" do you mean on an unmerged PR of yours, or a draft? I had set it to avoid you, but last night I flushed all my own guidance and told it to go with things each individual has said about their wishes and preferences. So you can set your own policy. Perhaps you'd prefer "don't talk about my stuff more than once a day" or "wait 72 hours before saying anything about my activities" or "never write more words to me than the word count you're responding to." All are interesting throttle ideas.

[ann0see]

No. He says that you should have probably not used AI at all. Or that the output is not concise enough.