PLAYER/wieldview: Fix server crash by client leaving after joining #138

erlehmann · 2021-08-22T04:31:53+02:00

erlehmann commented

2021-08-22 04:31:53 +02:00

Problem

TRACKING ISSUE: #104

When a player joins and immediately leaves the game before a function is
called by minetest.after() in mods/PLAYER/wieldview/init.lua, it gets an
invalidated player object. This results in the player methods returning
nil (since Minetest 5.2); perhaps surprisingly, the player is not nil.

Not checking that the result of player:get_pos() is not nil could lead
to a server crash if a client crashed when joining. It has been reported
that a syntax error in a client side mod was enough to trigger that.

Solution

If the result of player:get_pos() equals nil, the function returns early.

Details

The Minetest modding bookb by rubenwardy describes this as a common mistake:

Be Careful When Storing ObjectRefs (ie: players or entities)

An ObjectRef is invalidated when the player or entity it represents leaves the game. This may happen when the player goes offline, or the entity is unloaded or removed.

The methods of ObjectRefs will always return nil when invalid, since Minetest 5.2. Any call will essentially be ignored.

You should avoid storing ObjectRefs where possible. If you do to store an ObjectRef, you should make you check it before use, like so:
-- This only works in Minetest 5.2+
if obj:get_pos() then
	-- is valid!
end

https://rubenwardy.com/minetest_modding_book/en/quality/common_mistakes.html

Testing Steps

The game only crashes if a player manages to quit the server before a function is executed on join.

Since the crash involves a race condition, source code must be modified to win the race every time.

(“Winning the race” here means being able to quit a server before the crashy function is executed.)

Verify Bug

Create a new world in Minetest 5.4+ with Mineclonia commit 3cd4ad5591.
Change line 72 of mods/PLAYER/wieldview/init.lua from minetest.after(0, function(player) to minetest.after(10, function(player)
Start a Minetest server (check “host server”, but not “publish server”) at port 30000.
Using a second Minetest client, connect to the Minetest server at 127.0.0.1 port 30000.
As the second Minetest client finishes connecting to the server, immediately disconnect.
Verify that the Minetest server crashes in less than 10 seconds.
- The error message should contain “Invalid position (expected table got nil)”
- The crash should be triggered by line 74 of mods/PLAYER/wieldview/init.lua

Verify Patch

Create a new world in Minetest 5.4+ with Mineclonia commit 10ce37d887.
Change line 72 of mods/PLAYER/wieldview/init.lua from minetest.after(0, function(player) to minetest.after(10, function(player)
Start a Minetest server (check “host server”, but not “publish server”) at port 30000.
Using a second Minetest client, connect to the Minetest server at 127.0.0.1 port 30000.
As the second Minetest client finishes connecting to the server, immediately disconnect.
Verify that the Minetest server does not crash in less than 10 seconds.

To do

Fill out PR template
Write testing steps
Verify test plan

##### Problem TRACKING ISSUE: https://git.minetest.land/Mineclonia/Mineclonia/issues/104 When a player joins and immediately leaves the game before a function is called by `minetest.after()` in `mods/PLAYER/wieldview/init.lua`, it gets an invalidated player object. This results in the player methods returning `nil` (since Minetest 5.2); perhaps surprisingly, the player is not `nil`. Not checking that the result of `player:get_pos()` is not `nil` could lead to a server crash if a client crashed when joining. It has been reported that a syntax error in a client side mod was enough to trigger that. ##### Solution If the result of `player:get_pos()` equals `nil`, the function returns early. ##### Details The Minetest modding bookb by rubenwardy describes this as a common mistake: > Be Careful When Storing ObjectRefs (ie: players or entities) > > An ObjectRef is invalidated when the player or entity it represents leaves the game. This may happen when the player goes offline, or the entity is unloaded or removed. > > The methods of ObjectRefs will always return nil when invalid, since Minetest 5.2. Any call will essentially be ignored. > > You should avoid storing ObjectRefs where possible. If you do to store an ObjectRef, you should make you check it before use, like so: > > ``` > -- This only works in Minetest 5.2+ > if obj:get_pos() then > -- is valid! > end > ``` <https://rubenwardy.com/minetest_modding_book/en/quality/common_mistakes.html> ##### Testing Steps The game only crashes if a player manages to quit the server before a function is executed on join. Since the crash involves a race condition, source code must be modified to win the race every time. (“Winning the race” here means being able to quit a server before the crashy function is executed.) ###### Verify Bug 1. Create a new world in Minetest 5.4+ with Mineclonia commit 3cd4ad55919c2249ba91ba84e45d1e9c5242b4ad. 2. Change line 72 of `mods/PLAYER/wieldview/init.lua` from `minetest.after(0, function(player)` to `minetest.after(10, function(player)` 3. Start a Minetest server (check “host server”, but not “publish server”) at port 30000. 4. Using a second Minetest client, connect to the Minetest server at 127.0.0.1 port 30000. 5. As the second Minetest client finishes connecting to the server, immediately disconnect. 6. Verify that the Minetest server crashes in less than 10 seconds. * The error message should contain “Invalid position (expected table got nil)” * The crash should be triggered by line 74 of `mods/PLAYER/wieldview/init.lua` ###### Verify Patch 1. Create a new world in Minetest 5.4+ with Mineclonia commit 10ce37d887ff5272cc6574fb4f31d48535aa793d. 2. Change line 72 of `mods/PLAYER/wieldview/init.lua` from `minetest.after(0, function(player)` to `minetest.after(10, function(player)` 3. Start a Minetest server (check “host server”, but not “publish server”) at port 30000. 4. Using a second Minetest client, connect to the Minetest server at 127.0.0.1 port 30000. 5. As the second Minetest client finishes connecting to the server, immediately disconnect. 6. Verify that the Minetest server does not crash in less than 10 seconds. ##### To do - [x] Fill out PR template - [x] Write testing steps - [x] Verify test plan

erlehmann added 1 commit 2021-08-22 04:31:54 +02:00

10ce37d887

Fix server crash by client leaving after joining

When a player joins and immediately leaves the game before a function is
called by minetest.after() in mods/PLAYER/wieldview/init.lua, it gets an
invalidated player object. This results in the player methods returning
nil (since Minetest 5.2); perhaps surprisingly, the player is not nil.

Not checking that the result of player:get_pos() is not nil could lead
to a server crash if a client crashed when joining. It has been reported
that a syntax error in a client side mod was enough to trigger that.

erlehmann changed title from ~~WIP: Fix server crash by client leaving after joining~~ to WIP: PLAYER/wieldview: Fix server crash by client leaving after joining

2021-08-22 17:27:35 +02:00

erlehmann changed title from ~~WIP: PLAYER/wieldview: Fix server crash by client leaving after joining~~ to PLAYER/wieldview: Fix server crash by client leaving after joining

2021-08-22 19:09:51 +02:00