Bound kart-selection entries to peer profiles

[acts-1631]

ServerLobby::kartSelectionRequested and liveJoinRequest pass client-supplied kart lists to ServerLobby::setPlayerKarts. That helper reads the packet’s first byte as player_count, then indexes peer->getPlayerProfiles()[i] for every entry. The only existing check is that the profile list is nonempty.

A joined client can send more kart entries than it has profiles. The server then accesses a shared_ptr past the end of the vector, which can crash the authoritative game server during normal lobby or live-join handling.

This change rejects counts larger than the peer’s profile count before decoding or updating any kart data.

[Alayan-stk-2]

You removed the agreement message that fills the default PR description box. Please add it back.

[acts-1631]

You removed the agreement message that fills the default PR description box. Please add it back.

Hi Alayan. I was using github-cli to submit PRs rather than the web interface, so I did not see the agreement template. I am using AI for these bugs, with human review, so I can't lie and say otherwise. The diffs are very small and do not introduce any new features, so they would likely not constitute a copyrightable change even for a human. I still have a few remote server crash / cheating fix bugs in my forked repo to submit after (if) the current PRs are merged. If you'd prefer I don't submit them, let me know and feel free to close the pending ones. Sorry for the trouble.

[Alayan-stk-2]

Hi Alayan. I was using github-cli to submit PRs rather than the web interface, so I did not see the agreement template

Ok.

I am using AI for these bugs, with human review, so I can't lie and say otherwise.

Are you using it to help you analyze the code and find the potential issues? Using it for analysis is explicitly allowed in our code contribution policy, but simply copy-pasting whatever it suggests as a solution is not. The motivations for that are laid out at https://supertuxkart.net/How_to_contribute_code#supertuxkarts-policy-on-ai-generated-code

I already got multiple PRs made with AI that had verbose code and used a very roundabout way of trying to do something. That's unreviewable and goes directly against the goal of making the code simpler, clearer, and easier to maintain.

If you have given thought about the nature of what the change sets out to do, and if some of the code lines were not directly written by you, but follow your design idea and have been carefully reviewed, tested and understood so that you would be able to explain their purpose if I have a question, we get into an area that technically runs afoul of the rule-as-written but not of its intent (except for the copyright concern but as discussed below, for a change like this it doesn't matter). I'd also note that traditional software autocomplete tools also "write" some code; the policy is not about whether each brace or each letter of a variable was typed manually.

In this case, it certainly seems better to not set a kart to a non-existent player profile, trying to write in an incorrect array location. Buffer overflows are no-good.

However it leaves a few question opens.

• What would be the typical source of the issue? Another client with malicious code trying to crash regular clients?
• The function is just left early instead of trying to assign karts up to the safe/allowed number. Why that choice? Is that data re-sent at some point?

The diffs are very small and do not introduce any new features, so they would likely not constitute a copyrightable change even for a human.

That's correct. In my dual-licensing tracker, there are some contributors for which we don't have an explicit agreement which I've marked as ok simply because their contribution does not reach the required threshold of originality to be copyrightable. When there is an objective issue and a clear sensible way to fix it, the fix can't be copyrighted, so the AI licensing issue is moot for that specific case.

Still, it's cleaner if you explicitly allow the dual-licensing for whichever portion of your PRs you have authorship over and that might not be covered by this triviality/obviousness exception.

I still have a few remote server crash / cheating fix bugs in my forked repo to submit after (if) the current PRs are merged. If you'd prefer I don't submit them, let me know and feel free to close the pending ones. Sorry for the trouble.

If their scope is similar - an objective issue with a fairly objective fix - it's probably worth sharing. It's not as if OOB issues are something we want in the codebase.

[acts-1631]

For my setup, I have a custom methodology that requires three steps of verification and real-world practicality before the AI presents any finding to me. Each patch is also build-tested if possible. I'm manually reviewing the results in most cases and have definitely had to make some corrections or discard some findings that were not good.

The malformed packet would normally come from a modified client (not unrealistic since this is an open source game), although a buggy client could produce the same input.

Both ServerLobby::kartSelectionRequested and the non-spectator path in ServerLobby::liveJoinRequest pass a client-supplied count to setPlayerKarts(). A client with one profile can send two or more entries, causing the server to index past peer->getPlayerProfiles() and potentially take down the authoritative server for everyone connected to it.

The LLM chose (and I agreed) to reject the complete list rather than clamp the count. The packet contains all kart-name strings first, followed by optional KartData records for the same entries. Clamping the count before decoding would leave the extra names in the stream and cause the KartData parser to read from the wrong offset. Safely truncating it would require a broader parser change and I don't want to make too verbose of a change per your comment.

The normal client sends this message once from NetworkKartSelectionScreen::allPlayersDone(). For regular lobby selection, an empty kart selection is replaced with a random kart when the race is prepared. For live joining, the follow-up commit makes setPlayerKarts() report rejection so liveJoinRequest() stops and returns the peer to the lobby instead of continuing with unset kart data.

[Alayan-stk-2]

The packet contains all kart-name strings first, followed by optional KartData records for the same entries. Clamping the count before decoding would leave the extra names in the stream and cause the KartData parser to read from the wrong offset. Safely truncating it would require a broader parser change and I don't want to make too verbose of a change per your comment.

Ok, that makes a lot of sense.

Your new commit addresses the implicit problem I raised in my previous comment, namely that just leaving the function without doing anything else is probably not producing a safe network state. (And you can see why I bother reviewing even seemingly simple fixes! The first version was not quite sufficient to solve the problem)

For regular lobby selection, an empty kart selection is replaced with a random kart when the race is prepared.

Great, that simplifies things. I suppose that was already the case because some players are too slow to pick a kart, and straight up rejection on live-join requests is safe.

I noticed one small odd thing in the current PR code, but it's really trivial to handle.

Considering the code and our discussion, I intend to merge your AI-assisted code, however I wrote earlier:

Still, it's cleaner if you explicitly allow the dual-licensing for whichever portion of your PRs you have authorship over and that might not be covered by this triviality/obviousness exception.

Please provide your agreement for GPL3-MPL2 dual-licensing. If you state your agreement covers past and future code you submit to SuperTuxKart, there won't be a need to do it multiple times as it will also cover your other PRs.

[Alayan-stk-2]

Since this is a void function that would terminate anyway right after the call to setPlayerKarts, this doesn't do anything?

[acts-1631]

I agree that all past and future code contributions I submit to SuperTuxKart may be dual-licensed under the GPL version 3 or later and the MPL version 2 or later.

Is that okay?

And just removed the redundant return check in kartSelectionRequested().

[Alayan-stk-2]

I agree that all past and future code contributions I submit to SuperTuxKart may be dual-licensed under the GPL version 3 or later and the MPL version 2 or later.

Is that okay?

Yes, perfect.

And just removed the redundant return check in kartSelectionRequested().

Good