Bad actors will exploit RemoteFunction’s not having a configurable timeout by never returning - causing server-sided memory leaks.
Expected behavior
RemoteFunction should allow us to specify a timeout to prevent exploiters from crashing servers.
9 Likes
I’m curious. Do the stuck requests eventually error/return and clean up resources when the unresponsive client eventually disconnects? Or does the memory leak persist even after the unresponsive client disconnects?
If it’s the former then you can mitigate this type of attack by kicking the client if they take too long to respond.
But ideally you should redesign your system to not use remote functions invoked from the server. What is your use case?
3 Likes
Expensive 3D calculations that are infeasible to perform per player on the server real-time, also that use client only known state information. However it would be better to remove the functionality entirely by the logic of “redesign your system to never use a function” If the function is to remain it should be made to be robust.
3 Likes
RemoteFunction/InvokeClient really only stays around for backwards compatibility. The use cases you are describing here would be better off designed with RemoteEvents, as then the server would be running logic only when the clients send in these expensive calculations and state instead of polling for it.
I have never come across a point where I needed to actually use InvokeClient for something legit, and personally I think it should be deprecated to sway away misuse.
5 Likes
Can you give an example of where you would need to use InvokeClient, and where a delay would cause problems? This would make it easier to understand the severity of the issue.
2 Likes
As others have mentioned here, it’s a fundamentally flawed function that I would recommend migrating your code away from.
Even if you had a timeout, I think you would find that properly handling the case where the call timed out would be more complex than simply writing things using RemoteEvents to begin with. Asking the client to do something and give you a result doesn’t conceptually work well when you can’t trust the client to do that task correctly… not to mention other awkward mechanics such as the fact that the client could legitimately disconnect halfway through the call complicating the timing considerations of the invoking code.
4 Likes
I am not concerned whether the client performs the task correctly as it can be checked. There exists problems that require significant computation to solve but can be quickly checked for validity (NP). =]
The use case is calculating an available position to spawn an egg around the character that is not obstructed by any parts/players/pets/other eggs/etc. The hard part is finding the ideal location that looks nice. The server cannot perform this calculation for many players and many eggs. The server however can EASILY check if the returned position information is not obstructed and is nearby the player.
This use case is currently in production. I got around players who were never returning the results by some very complicated coroutine logic, but I am unsure if that is robust and seems to leave the ‘thread’ in an undefined behavior state that may be leaking objects.
e.g.:
local remoteFunction = ...
local results = tcall(10, function()
return remoteFunction:InvokeClient(player)
end)
function tcall<P..., R...>(
timeout: number,
fn: (P...) -> (R...),
...: P...
): (boolean, R...)
local errMessage: string? = nil
local errStackTrace: string? = nil
local results: {any}? = nil
local wrappedFn = function(...: P...)
results = table.pack(xpcall(fn, function(err: any?)
errMessage = tostring(err)
errStackTrace = tostring(debug.traceback(nil, 3))
end, ...))
end
local co = coroutine.create(wrappedFn)
local startTime = os.clock()
local coResults = table.pack(coroutine.resume(co, ...))
if not coResults[1] then
error(table.unpack(coResults::{any}, 2))
end
while true do
local status = coroutine.status(co)
if results then
if status ~= "dead" then
coResults = table.pack(coroutine.resume(co))
coroutine.close(co)
if coResults[1] ~= true then
error(table.unpack(coResults::{any}, 2))
end
end
coroutine.close(co)
local success = results[1]
if success ~= true then
error(`{tostring(errMessage)}\nStack Begin\n{tostring(errStackTrace)}Stack End`, 0)
end
return success, table.unpack(results, 2)
end
if status == "dead" then
coResults = table.pack(coroutine.resume(co))
coroutine.close(co)
if coResults[1] ~= true then
error(table.unpack(coResults::{any}, 2))
end
error("died")
end
if os.clock() - startTime >= timeout then
coroutine.close(co)
return false
end
task.wait()
end
end
This is needlessly complicated to achieve this goal lol, however this function is useful for certain things which cannot be trusted in terms of running time.
1 Like
Closing a coroutine that is yielding for RemoteFunction:InvokeClient() will not clean up any internal engine resources that are waiting for a response from that specific client. This is my best guess so if I’m wrong about that then I would like to know. That’s why I suggested kicking unresponsive clients, since that will probably clean up any resources the engine is using internally to wait for a client response (If this is not the case then that’s an engine issue).
Ditching InvokeClient() is still the way you should go.
Can you could describe how eggs work in your game?
Yeah so my theory is that the attacker is spamming a remote to open a bunch of eggs but ignores the request which is sent back to them for the egg position. This will cause a bunch of threads to pile up on the server when InvokeClient() never returns. If there are no checks in place to ignore requests to open eggs that are currently being processed then this is even more potent.
You have to remove that request which goes back to the client. I suggest you move all the egg positioning code to the server. If you still think the egg positioning code is too expensive for the server you can still calculate if from the client but send it along with the egg opening request.
Why is the egg positioning code so expensive?
It requires a lot of ray trace Raycast / GetPartBoundsInBox invocations such that would be impractical to run on the server continuously for the amount of eggs that are being opened. There exists an O(n log n) algorithm to calculate the ideal locations, but it would require too much time to develop and get perfectly right. The naive algorithm takes much less time to write and is also is accurate without investing hours into fixing corner cases. (Better things to do!)
1 Like
Another easy thing to do would be to spread the search time out over multiple frames. It would probably even take less time than it does for InvokeClient() to return in your current setup.
I agree with the OP, both methods (InvokeClient and InvokeServer) should have a timeout.
Making the excuse that InvokeClient is not to be trusted on doesn’t justify the reason for this not to be added. If you don’t want people to be using InvokeClient (because it seems so in your post, @tnavarts), then maybe just deprecate it. If this were not to be added, then we would have to find a hacky, bad practice solution using remotes.
(My post has nothing to do with the egg positioning of OP’s game, I just realized it should be a feature)
3 Likes