Skip to content

Fix bullet sync validation bypass - #5115

Open
QueryOfficial wants to merge 1 commit into
multitheftauto:masterfrom
QueryOfficial:fix/bulletsync-validation-bypass
Open

Fix bullet sync validation bypass#5115
QueryOfficial wants to merge 1 commit into
multitheftauto:masterfrom
QueryOfficial:fix/bulletsync-validation-bypass

Conversation

@QueryOfficial

@QueryOfficial QueryOfficial commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Bullet sync packets were relayed to nearby players from the sync thread by
CSimPlayerManager::HandleBulletSync, which validated only CVector::IsValid() on the
start and end positions. The main thread path (CBulletsyncPacket::Read
CGame::Packet_Bulletsync) applies far stricter checks, but it runs after the sync
thread has already broadcast the packet — and a packet rejected there is dropped
silently. A shot could therefore reach every nearby client while onPlayerWeaponFire
never fired for it, leaving the scripting layer with no record of it.

CVector::IsValid() only rejects NaN and infinity. A coordinate such as 0xFFFFFFFF
reinterpreted 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:

  • adds Shared/mods/deathmatch/logic/CBulletSyncValidation.h, a dependency free set of
    checks 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
  • moves all bullet sync validation into CBulletsyncPacket::Read, so nothing is relayed
    before it has passed
  • stops relaying bullet sync from the sync thread (HasSimHandler() now returns false),
    so every shot goes through CGame::Packet_Bulletsync and fires onPlayerWeaponFire
    before it is broadcast
  • applies the same trajectory bounds to CCustomWeaponBulletSyncPacket and to the
    receiving side in CNetAPI
  • adds 24 unit tests covering the validator

Existing 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 onPlayerWeaponFire stopped firing, while the sync thread had already delivered the
same 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: CSimPlayer is
a 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:

  • Thread safety. CBulletsyncPacket::Read() runs on the main thread:
    CNetServerBuffer::ProcessPacket queues the packet on the sync thread and
    CNetServerBuffer::ProcessIncoming invokes the packet handler during the main pulse.
    The new g_pGame->GetWeaponStatManager() and CPlayer::GetPosition() calls are
    therefore safe. The existing checks in that function already relied on this.
  • Range tolerance. The previous check was 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 widens
    by about 5%. This is deliberate; say the word if you would rather keep the old value.

Test plan

Automated, run by the existing Tests workflow:

  • 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 maxRange values
    (NaN / infinity / negative) that must not widen the envelope, muzzle distance on foot
    and in a vehicle, and the damage/hit zone payload.
  • Regression cases assert that a payload built around a huge-but-finite coordinate is
    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.
  • One case asserts EXPECT_TRUE(start.IsValid()) on the crafted position, documenting
    why the previous check was not sufficient.

Local results: 24/24 new, 328/328 total. utils/clang-format.ps1 reports no violations.
Server and client sources compile clean (Deathmatch and Client 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:

  1. Confirm normal combat is unaffected — on foot and in a vehicle, all bullet sync
    weapons (22-34), including sniper at long range.
  2. Confirm onPlayerWeaponFire now fires for every shot other players can see.
  3. Watch main thread load on a busy server: bullet relay moved off the sync thread, so
    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.h with a test beside
the existing ones, rather than in the packet classes.

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.

@Dryxio

Dryxio commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

"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
weapons (22-34), including sniper at long range.
Confirm onPlayerWeaponFire now fires for every shot other players can see."

So these changes were not tested ingame?

@QueryOfficial

Copy link
Copy Markdown
Contributor Author

I did test it locally and combat behaves normally on my end, with onPlayerWeaponFire firing as expected.

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 CGame::Packet_Bulletsync if that would make the comparison measurable.

I've reworded the heading of that section in the description to say exactly this; the items themselves are unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants