Fix push sync vehicle syncer hijack

[Zephkek]

Summary

Prevent vehicle push sync from replacing an existing unoccupied vehicle syncer.

Conditions for whether a player can claim syncer of a vehicle:

• there is no existing syncer
• there is no "persistent syncer" (set when setElementSyncer is used)
• there is a contact element

Motivation

Pushsync accepted a nearby client's vehicle ID and could call OverrideSyncer even when another player already owned sync. This let forged pushsync packets hijack vehicle sync authority.

Test plan

• Review the pushsync path to confirm it now only fills missing syncers.

Checklist

• Your code should follow the coding guidelines.
• Smaller pull requests are easier to review. If your pull request is beefy, your pull request should be reviewable commit-by-commit.

[Lpsd]

Very nice, thanks!

[qaisjp]

Is this the server side equivalent of this check?

mtasa-blue/Client/mods/deathmatch/logic/CClientGame.cpp

Lines 4710 to 4713 in 6c54a2d

	// Sync Stuff
	// if it's not a local vehicle + it collided with the local player
	if (pVehicleClientEntity->IsLocalEntity() == false && pCollidedWithClientEntity == g_pClientGame->GetLocalPlayer())
	{

[qaisjp]

yep -- 6df1094

[qaisjp]

This affects sync when the player ped is "pushing" a vehicle, by walking into it.

pPlayer->GetContactElement() == pVehicle is the main change here.

This could in theory cause 1.5s of desync for a player vehicle, if the "contact" packet arrives after the vehicle push packet.

If that happens, the server will reject the syncer change and the client will stop sending syncer packets for ~1.5s (because of the anti-span check client side)

This seems OK given that:

• if there are other players in the vicinity, the other players will have their own simulation of the push
• the player doing the pushing is likely pushing for more than 1.5s, so the next push packet is likely to succeed
• if the player is pushing a vehicle carefully (where packets being lost for 1.5s would be bad), they have likely already set foot in the vehicle before

That said, is this change strictly necessary, given that we have the iVehicleContactSyncRadius check?

[qaisjp]

Question for you @Zephkek!

[Zephkek]

@qaisjp Thanks for the review, to answer your question yes I believe this check is necessary because the radius check alone doesn't prevent a cheating client from hijacking sync (default 30 units which is about 4 car lengths). This PR fixes the ability to forge or unintended push packet from a player who is near a vehicle but not actually pushing it so radius alone passes that. GetContactElement() == pVehicle is checking if the player is actually in contact with the vehicle and gate sourced from the independent puresync stream, which the push packet can't unilaterally fake.

So the two checks aren't redundant the contact check is what specifically prevents syncer hijack and it's only enforced when there's already a syncer to take over from so the no-syncer "first one to nudge it" remains unchanged as intended.

[Zephkek]

If that happens, the server will reject the syncer change and the client will stop sending syncer packets for ~1.5s (because of the anti-span check client side)

Also about the 1.5s desync concern, in theory yes client can lose 1.5s of push sync packets if the contact element update arrives after the first push packet but that window is bounded by exactly the case where it least matters:

• It only triggers when another player is already the syncer of the same vehicles otherwise (!pSyncer bypasses the contact check) meaning someone elses simulation is already authoritative so visible drift is minimal

• Persistent syncers bypass it via !IsSyncerPersistent()

• For non persistent cases with an existing syncer, the legitimate displacing player will set foot on/ collide with the vehicle within a frame or two of starting to push and the next push packet after MIN_PUSH_ANTISPAM_RATE will go through normally