SetPos Exploit Fix

I keep getting asked to fix this issue and I haven’t until now because it really shouldn’t be the users job to fix a exploit in the physics engine. However, Garry has made it clear he has no intention of fixing the issues when it was brought up last time. The problem is solved by first making sure the prop is not going to move further then the engine can handle. The default is 500,000 units and that is plenty but close enough to prevent the crash. The script isn’t exactly complicated and the source is a pastebin below. It’s a good idea to run this both client and server because of the potential for a client crash from ghosted props.

This overwrites the SetPos functions for entities and phyobjects.

To test make a prop move really far. I used the precision tool after running “precision_offset 9999999999999999999999” in console. Use the move setting and it should cause some issues.

Also if you find any more server crash exploits, send me a pm or if you really want to make then public just make a reply.

Stay Up-To-Date: http://douglashuck.com/
Addon Direct DL: http://douglashuck.com/files/gmod_prevent_setpos_crash.zip
PasteBin: http://pastebin.com/1hCaY1Qv
Workshop: http://steamcommunity.com/sharedfiles/filedetails/?id=274993058

Current Version 1.4.0

I’d recommend localizing the functions you’re calling like math.Clamp etc since you’re overriding two functions that get called ALOT.
Secondly rather than use getconvar you should probably add an on change hook to the convar and cache the value when it changes.
Though it’s great you fixed an exploit the fix you’ve shown will dramatically reduce performance in code that calls those functions a lot.

Pretty much what was said above. Doing 6 convar lookups is extremely slow.

Regardless of that, what was the point of this script? Why not just fix the scripts with the “exploit”? It’s been pretty common knowledge that if an entity goes too far outside the map the game will crash or at least crazy physics will remove it.

It’s been updated. Thank you.

Crazy physics removes it to a limit. If you go to far it simply crashes and with alot of stools having no validation or limits, you can make it go as far as needed to cause a crash. This isn’t a new problem, I fixed stacker and precision before but they refused to include the changes to have a limit.

Simply this fixes it, for everything, for good. You really shouldn’t feel any performance changes, if you are setting the props position so much that you do feel it you shouldn’t need this because you are not running sandbox or you should look into fixing the tools yourself so that this isn’t needed.

Edit:
I tried to measure the performance of the script.

Using the precision tool and this code:



local startTime = os.clock()
local bound = GetConVar("preventcrash_bounds"):GetFloat()
pos.x = clamp(pos.x, 0-bound, bound)
pos.y = clamp(pos.y, 0-bound, bound)
pos.z = clamp(pos.z, 0-bound, bound)
print(os.clock()-startTime)


http://puu.sh/8jtSs.jpg

Firstly use SysTime it’s unpredicted so it will give you more accurate benchmark results than os.time secondly even if it is really fast seeming it’s still pretty slow to be honest. The goal for any frequently called function is to be as fast as it can possibly be made so that when you call it in your code you can worry about your algorithm’s speed and not how much time will be used by calling some library function or object method.

Currently the algorithm has to do something like the following (might be a bit off) in terms of virtual machine code

Index global variable table for GetConVar and put on stack
Push string <cvar name>
Call function returns Cuserdata
index CUserData for method GetFloat
Call C function passing userdata
Allocate and assign local bound to resulting double

And that’s just for getting the convar… Pretty ugly when you think of it (and that’s at a higher level than the actual instruction set that’s getting generated) but it’s still pretty bad. And when you consider that calling methods on C types requires indexing a C++ metatable etc it’s just bad bad bad.

I’d really recommend you replace your convar with a constant for something like this since the convar serves little purpose and there are few reasons anyone would want or need to change it if you set it to a reasonable value (note that a constant, not a variable, since constants are even faster)
I’d also suggest using if statements instead of math.clamp to avoid the function call overhead and also avoid the pointless variable assignment if the value doesn’t need to be modified.

Also considder making the original function a local variable and calling it as OldSetPos( self, pos ) as this is faster than indexing the metatable again.

This would give you completely optimal code with minimal performance reductions. This is particularly important for stuff like the DarkRP chat text things which continually set their positions above players aswell as a ton of scripts which use setpos every frame client side for ghosting potentially large groups of entities.

Never knew of the SysTime Function, here’s the break down.
Clamp: 5.650590537698e-006 Total:1.4691535625388e-005 Percent Increase:38.461537866288

So we get 0.000005650590537698 of a second time to execute the clamp and that is 38% of the total time it to to run the SetPos function in full including the pass to the old function.

I tried if statements in every configuration I could think of and they are not faster then the math.clamp and I assume this is because math.clamp is a native lua library and might be actually optimized c.

I guess servers operators will have to pick between six millionths of a second, fixing every exploitable version of SetPos, or leaving the exploit open.

I’d recommend making it autorefresh compatible. Right now if you autorefresh with it you’ll reset the Ent.SetRealPos with the current SetPos which you overwrote…

Only set it if it doesn’t exist…

Here’s what I do for a few:

[lua]if ( !PRINT ) then PRINT = print; end

// When I made this fix, I did this
– if ( !PARTICLE_EMITTER ) then PARTICLE_EMITTER = ParticleEmitter; end

// New emitter fix does this
if ( !META_EMITTER.__Finish ) then META_EMITTER.__Finish = META_EMITTER.Finish; end

// and

if ( !__GLOBAL_PARTICLE_EMITTER ) then __GLOBAL_PARTICLE_EMITTER = { }; end[/lua]

You get the idea; it’s pretty straightforward to implement auto-refresh compatibility.

Fixed.

This may be a bump (found the thread from Google), but is an if statement better practice than doing
[lua]
Ent.SetRealPos = Ent.SetRealPos or Ent.SetPos
Phys.SetRealPos = Phys.SetRealPos or Phys.SetPos
[/lua]

That’s how I’ve always done it, which is better in general? Seems cleaner, and in theory should run faster (not by much, but… ye).

It’s personal preference. There are speed differences (I don’t know what is faster off the top of my head) but it is negligible. I prefer to use if-statements.

Those lines are only ran one time so the speed difference isn’t really something to consider here but I do like how the two lines look compared to 6 so I changed it, thanks for the suggestion.

I also added this to the workshop to make it easier to add to servers using workshop files already.