Skip to content

fix: malformed packet crash #889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
May 19, 2025
Merged

fix: malformed packet crash #889

merged 6 commits into from
May 19, 2025

Conversation

TestingPlant
Copy link
Collaborator

When a client sends a malformed packet, the server will crash because it used entity.destruct to disconnect players for malformed packets which leads to an abort because, when the server receives PlayerDisconnect from the proxy, the server will try to set PendingRemove on the already-destructed entity.

This PR fixes these errors on player disconnects.

@github-actions github-actions bot added the fix label Apr 27, 2025
@github-actions GitHub Actions
Copy link

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  33.9 ns ...  33.8 ns ]      -0.28%
ray_intersection/aabb_size_1                       [  33.8 ns ...  33.6 ns ]      -0.52%
ray_intersection/aabb_size_10                      [  24.4 ns ...  24.4 ns ]      +0.30%
ray_intersection/ray_distance_1                    [  13.8 ns ...  13.8 ns ]      -0.09%
ray_intersection/ray_distance_5                    [  13.8 ns ...  13.8 ns ]      -0.05%
ray_intersection/ray_distance_20                   [  13.8 ns ...  13.8 ns ]      -0.13%
overlap/no_overlap                                 [  24.7 ns ...  24.8 ns ]      +0.27%
overlap/partial_overlap                            [  25.0 ns ...  25.4 ns ]      +1.47%
overlap/full_containment                           [  22.5 ns ...  22.5 ns ]      +0.15%
point_containment/inside                           [   5.5 ns ...   5.0 ns ]     -10.08%
point_containment/outside                          [   4.4 ns ...   4.4 ns ]      +0.13%
point_containment/boundary                         [   4.9 ns ...   4.9 ns ]      +0.19%

Comparing to 4c402d0

@codecov Codecov
Copy link

codecov bot commented Apr 27, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 4.34783% with 44 lines in your changes missing coverage. Please review.

Project coverage is 20.84%. Comparing base (347728c) to head (46eed2e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/hyperion/src/net/mod.rs 0.00% 19 Missing ⚠️
crates/hyperion/src/ingress/mod.rs 25.00% 6 Missing ⚠️
crates/hyperion-proxy/src/egress.rs 0.00% 5 Missing ⚠️
crates/hyperion-proxy/src/player.rs 0.00% 5 Missing ⚠️
crates/hyperion-proxy/src/cache.rs 0.00% 3 Missing ⚠️
crates/hyperion-proxy/src/data.rs 0.00% 3 Missing ⚠️
crates/hyperion/src/egress/player_join/mod.rs 0.00% 2 Missing ⚠️
crates/hyperion-proto/src/server_to_proxy.rs 0.00% 1 Missing ⚠️ 
@@           Coverage Diff           @@
##             main     #889   +/-   ##
=======================================
  Coverage   20.84%   20.84%           
=======================================
  Files         161      161           
  Lines       16870    16864    -6     
  Branches      469      464    -5     
=======================================
- Hits         3517     3516    -1     
+ Misses      13289    13284    -5     
  Partials       64       64
Files with missing lines Coverage Δ
crates/hyperion-proto/src/server_to_proxy.rs 12.50% <0.00%> (-1.79%) ⬇️
crates/hyperion/src/egress/player_join/mod.rs 29.05% <0.00%> (-0.13%) ⬇️
crates/hyperion-proxy/src/cache.rs 0.00% <0.00%> (ø)
crates/hyperion-proxy/src/data.rs 0.00% <0.00%> (ø)
crates/hyperion-proxy/src/egress.rs 0.00% <0.00%> (ø)
crates/hyperion-proxy/src/player.rs 0.00% <0.00%> (ø)
crates/hyperion/src/ingress/mod.rs 22.83% <25.00%> (+0.66%) ⬆️
crates/hyperion/src/net/mod.rs 6.60% <0.00%> (-0.31%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

andrewgazelka
andrewgazelka previously requested changes Apr 27, 2025
View reviewed changes
Copy link
Member

@andrewgazelka andrewgazelka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good. However, requires a couple of changes.

@github-actions GitHub Actions
Copy link

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  33.5 ns ...  33.4 ns ]      -0.27%
ray_intersection/aabb_size_1                       [  33.9 ns ...  33.8 ns ]      -0.24%
ray_intersection/aabb_size_10                      [  24.3 ns ...  24.3 ns ]      -0.18%
ray_intersection/ray_distance_1                    [  13.8 ns ...  13.8 ns ]      +0.09%
ray_intersection/ray_distance_5                    [  13.8 ns ...  13.8 ns ]      -0.19%
ray_intersection/ray_distance_20                   [  13.8 ns ...  13.8 ns ]      -0.03%
overlap/no_overlap                                 [  24.9 ns ...  24.8 ns ]      -0.36%
overlap/partial_overlap                            [  25.0 ns ...  25.1 ns ]      +0.35%
overlap/full_containment                           [  22.5 ns ...  22.5 ns ]      -0.10%
point_containment/inside                           [   4.9 ns ...   4.9 ns ]      -0.13%
point_containment/outside                          [   4.4 ns ...   4.4 ns ]      -0.30%
point_containment/boundary                         [   5.0 ns ...   4.9 ns ]      -0.21%

Comparing to 4c402d0

@github-actions GitHub Actions
Copy link

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  33.9 ns ...  33.8 ns ]      -0.09%
ray_intersection/aabb_size_1                       [  33.7 ns ...  33.8 ns ]      +0.17%
ray_intersection/aabb_size_10                      [  24.0 ns ...  24.0 ns ]      +0.02%
ray_intersection/ray_distance_1                    [  13.8 ns ...  13.8 ns ]      +0.19%
ray_intersection/ray_distance_5                    [  13.8 ns ...  13.8 ns ]      -0.21%
ray_intersection/ray_distance_20                   [  13.8 ns ...  13.8 ns ]      +0.04%
overlap/no_overlap                                 [  24.8 ns ...  24.8 ns ]      -0.04%
overlap/partial_overlap                            [  25.0 ns ...  25.2 ns ]      +0.57%
overlap/full_containment                           [  22.5 ns ...  22.5 ns ]      -0.09%
point_containment/inside                           [   4.9 ns ...   4.9 ns ]      +0.10%
point_containment/outside                          [   4.4 ns ...   4.4 ns ]      +0.04%
point_containment/boundary                         [   5.0 ns ...   5.0 ns ]      -0.05%

Comparing to 4c402d0

@TestingPlant
fix(proxy): remove player in shutdown
764bee3 
This prevents possible bugs from forgetting to remove the player from
the player reigstry when calling shutdown. It also removes the player
from the positions map.
@TestingPlant
fix: disconnect players properly
5681268 
Using entity.destruct to disconnect players leads to an abort because,
when the server receives PlayerDisconnect from the proxy, the server
will try to set PendingRemove on the already-destructed entity,
causing an abort.

Sending DisconnectS2c was removed because it is not possible to send
packets to disconnected players.
@TestingPlant
refactor(proxy): remove shutdown message
1756f3f 
The channel is closed which makes the recv loop end. Therefore, sending
a shutdown message is not needed.
@TestingPlant
refactor(net): use with(id::<PendingRemove>())
4903787
@TestingPlant
docs(hyperion-proto): fix PlayerDisconnect link
831b05f
@TestingPlant
fix: make PendingRemove zero size
46eed2e
@TestingPlant TestingPlant force-pushed the fix-malformed-packet-crash branch from 970c342 to 46eed2e Compare May 18, 2025 03:48
@github-actions GitHub Actions
Copy link

Benchmark Results for general

ray_intersection/aabb_size_0.1                     [  34.7 ns ...  34.6 ns ]      -0.18%
ray_intersection/aabb_size_1                       [  34.2 ns ...  34.8 ns ]      +1.63%
ray_intersection/aabb_size_10                      [  24.4 ns ...  24.4 ns ]      +0.03%
ray_intersection/ray_distance_1                    [  13.8 ns ...  13.8 ns ]      -0.05%
ray_intersection/ray_distance_5                    [  13.8 ns ...  13.8 ns ]      -0.04%
ray_intersection/ray_distance_20                   [  13.8 ns ...  13.8 ns ]      +0.07%
overlap/no_overlap                                 [  25.2 ns ...  25.2 ns ]      -0.03%
overlap/partial_overlap                            [  25.2 ns ...  25.2 ns ]      +0.21%
overlap/full_containment                           [  22.9 ns ...  22.9 ns ]      +0.05%
point_containment/inside                           [   5.1 ns ...   5.1 ns ]      -0.03%
point_containment/outside                          [   4.4 ns ...   4.4 ns ]      -0.14%
point_containment/boundary                         [   4.9 ns ...   4.9 ns ]      +0.06%

Comparing to 347728c

@TestingPlant TestingPlant dismissed andrewgazelka’s stale review May 19, 2025 02:58

All requested changes have been made and Andrew currently seems to be inactive in the project

@TestingPlant TestingPlant added this pull request to the merge queue May 19, 2025
Merged via the queue into main with commit 188a3e4 May 19, 2025
10 of 11 checks passed
@TestingPlant TestingPlant deleted the fix-malformed-packet-crash branch May 19, 2025 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewgazelka andrewgazelka andrewgazelka left review comments

@dyc3 dyc3 dyc3 approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TestingPlant @dyc3 @andrewgazelka