Zeex / sampgdk

Write SA-MP gamemodes in C/C++
http://zeex.github.io/sampgdk
Apache License 2.0
153 stars 83 forks source link

Fix invoke_array output string handling #218

Closed plutalov closed 2 years ago

plutalov commented 2 years ago

Push output params into fakeamx heap in the same way as individual natives do

The old way led to crashes

A bit about crashes themselves. I experienced behavior that was difficult to reproduce. Calling a native function that is supposed to return a string crashed the server. It didn't happen after a particular amount of callings but after some non-deterministic amount. I repeatedly (every 1 sec) called GetPlayerName and GetAnimationName natives via sampgdk_native_invoke_array. Moreover, that happened only during the first server launch after the OS (Windows 10) startup. Sometimes it happened during the subsequent launches but after the third, it constantly disappeared. I never discovered the other patterns though I came up with a solution.

Your GetPlayerName and GetAnimationName c++ functions didn't crash the server this way so I just changed the way we push output variables into fakeamx heap. That helped.

The only thing I have doubts about is the return value. For example, if I call GetPlayerName with playerid that has never been connected, I get some nonsense value. If the player was connected but then disconnected, it keeps returning his name. Though GetPlayerName C++ function has the same behavior.

Zeex commented 2 years ago

Thanks for the detailed explanation.

I see that you replaced the call to sampgdk_fakeamx_push_array() with sampgdk_fakeamx_push(). Those functions are very similar, in fact push_array calls the push function underneath, but the first one additionally copies the destination array to the heap, in case if the native function expects it to contain some input.

If using push instead of push_array works for you, I suspect that you might be passing an incorrect array size when invoking a native somewhere, which leads to a buffer overflow and corruption of AMX heap data. That's the only explanation that I can think of... If you have a stack trace of the crashes, it could provide some context. But right now this is just pure guessing.

I cannot merge this pull request because it removes part of the existing (seemingly working) functionality, sorry. There might be a way to add bounds checking to such native function calls to detect buffer overflows, similar to how various C(++) runtime do it in debug builds. I'd suggest to try looking into that direction.