Fix bullet sync validation bypass - #5115
Conversation
|
"Not covered by the above, and worth a look from someone with a live server: Confirm normal combat is unaffected — on foot and in a vehicle, all bullet sync So these changes were not tested ingame? |
|
I did test it locally and combat behaves normally on my end, with That list is what one machine can't answer, not what I skipped. Bullet sync is only ever relayed to other players, and item 3 (relay cost now paid during the main pulse) scales with concurrent shooters and tick load — neither of those shows up at ~0 ms ping with a couple of clients. So it wants a populated server before anyone takes my word for it, and I'm happy to add temporary instrumentation around I've reworded the heading of that section in the description to say exactly this; the items themselves are unchanged. |
Summary
Bullet sync packets were relayed to nearby players from the sync thread by
CSimPlayerManager::HandleBulletSync, which validated onlyCVector::IsValid()on thestart and end positions. The main thread path (
CBulletsyncPacket::Read→CGame::Packet_Bulletsync) applies far stricter checks, but it runs after the syncthread has already broadcast the packet — and a packet rejected there is dropped
silently. A shot could therefore reach every nearby client while
onPlayerWeaponFirenever fired for it, leaving the scripting layer with no record of it.
CVector::IsValid()only rejects NaN and infinity. A coordinate such as0xFFFFFFFFreinterpreted as a float is finite (~4.29e9) and passes, so out of range positions were
relayed and reached
CWeapon::FireBullet()on the receiving clients.This pull request:
Shared/mods/deathmatch/logic/CBulletSyncValidation.h, a dependency free set ofchecks used by both sides: absolute position bounds, trajectory length against the
weapon's own range, muzzle distance from the shooter, and the optional damage payload
CBulletsyncPacket::Read, so nothing is relayedbefore it has passed
HasSimHandler()now returnsfalse),so every shot goes through
CGame::Packet_Bulletsyncand firesonPlayerWeaponFirebefore it is broadcast
CCustomWeaponBulletSyncPacketand to thereceiving side in
CNetAPIExisting sources lose 306 lines and gain 77; the rest is the new shared header and its
test suite.
Motivation
The two relay paths had drifted apart. Hardening the main thread path alone made the
problem harder to observe rather than fixing it: the stricter check rejected the packet,
so
onPlayerWeaponFirestopped firing, while the sync thread had already delivered thesame packet to the closest viewers. Keeping a single path removes the class of bug
rather than one instance of it.
The sync thread could not perform the decisive check even in principle:
CSimPlayerisa snapshot that does not carry the player's position, so the muzzle origin cannot be
compared there without extending and maintaining that snapshot. Bullet sync was also
only half handled by the sim system — it relayed to zone 0 viewers while zone 1/2 already
went through the main thread, which meant the same shot reached different observers by
different paths with different latency.
I could not find an open issue that tracks this. #4497 ("Bullet sync / onPlayerWeaponFire
is still broken", closed) reports the same observable symptom — the event firing
inconsistently depending on location — so it may be worth revisiting once this lands.
Two notes for reviewers:
CBulletsyncPacket::Read()runs on the main thread:CNetServerBuffer::ProcessPacketqueues the packet on the sync thread andCNetServerBuffer::ProcessIncominginvokes the packet handler during the main pulse.The new
g_pGame->GetWeaponStatManager()andCPlayer::GetPosition()calls aretherefore safe. The existing checks in that function already relied on this.
distanceSq > rangeSq * 1.1, which is a~4.9% tolerance on distance despite the comment saying 10%. The shared validator
implements the documented intent (
(range * 1.1)²), so the accepted envelope widensby about 5%. This is deliberate; say the word if you would rather keep the old value.
Test plan
Automated, run by the existing
Testsworkflow:Tests/client/CBulletSyncValidation_Tests.cpp— 24 cases covering position bounds(NaN, infinity, huge finite values, the inclusive limit), trajectory length against
weapon range, the fallback cap when the range is unknown, hostile
maxRangevalues(NaN / infinity / negative) that must not widen the envelope, muzzle distance on foot
and in a vehicle, and the damage/hit zone payload.
rejected, and — importantly — that an ordinary shot fired from the same distant part of
the map is still accepted, so the bounds are relative to the shooter rather than to a
magic number.
EXPECT_TRUE(start.IsValid())on the crafted position, documentingwhy the previous check was not sufficient.
Local results: 24/24 new, 328/328 total.
utils/clang-format.ps1reports no violations.Server and client sources compile clean (
DeathmatchandClient Deathmatch, x86 Debug).Requires a populated server (not reproducible single-client):
Bullet sync is only ever relayed to other players, so the following cannot be observed
from a single client and are listed here for anyone able to run this build with real
traffic:
weapons (22-34), including sniper at long range.
onPlayerWeaponFirenow fires for every shot other players can see.the cost of broadcasting bullets is now paid during the main pulse.
For anyone changing this code later: the validator is pure and has no engine
dependencies, so any new bound belongs in
CBulletSyncValidation.hwith a test besidethe existing ones, rather than in the packet classes.
Checklist