Fix #5173: prevent server crash on invalid elementData argument

[MohabCodeX]

Summary

Refactor CScriptArgReader error handling to use LuaFunctionError exceptions instead of calling luaL_error directly.

Motivation

Fixes #5173.

Calling luaL_error while C++ objects are still on the stack can skip their destructors because Lua uses longjmp for error handling. This can cause memory leaks, heap corruption, or crashes.

The idea here is to let C++ handle the stack unwinding first, then report the error to Lua.

In short:

1. CScriptArgReader::ThrowError() throws LuaFunctionError.
2. C++ cleans up local objects normally.
3. CLuaFunctionParser catches the exception and calls luaL_error once the stack is clean.

Test Plan

arg_reader_suite.zip

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[FileEX]

This doesn't address the root cause of the problem - it's just a workaround, and an incomplete one at that. luaL_error is also used in many other files, so calling it in any other way will cause the same crash. Even the argument parser uses luaL_error.

[MohabCodeX]

This doesn't address the root cause of the problem - it's just a workaround, and an incomplete one at that. luaL_error is also used in many other files, so calling it in any other way will cause the same crash. Even the argument parser uses luaL_error.

If I add a safe helper method inside CScriptArgReader:

int ThrowError()
{
    return luaL_error(m_luaVM, "%s", GetFullErrorMessage().c_str());
}

And replace the raw luaL_error argument error calls with argStream.ThrowError(), would that be the right direction to fix this systemically?

[FileEX]

The key point isn't where luaL_error() is located, but whether there are still C++ objects on the stack that require destruction when it is called. Until the issue with MTA closing when debugging is fixed, I can't help with it

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[MohabCodeX]

he key point isn't where luaL_error() is located, but whether there are still C++ objects on the stack that require destruction when it is called. Until the issue with MTA closing when debugging is fixed, I can't help with it

Makes sense, thanks 🤍
I updated CScriptArgReader::ThrowError() to throw LuaFunctionError instead, so the stack unwinds before luaL_error is called at the outer parser boundary.

[FileEX]

Inside ThrowError, calling GetFullErrorMessage creates a temporary SString object on the heap, so this doesn't actually solve the problem properly (even if it doesn't crash in this particular case).

[MohabCodeX]

I see what you mean, but since we're throwing a standard C++ exception here instead of calling luaL_error directly, exception unwinding actually guarantees that the temporary SString's destructor is called right away before the stack leaves the frame. So unlike luaL_error's longjmp, it won't leak or bypass any destructors.

That said, if you'd still prefer avoiding that temporary SString allocation inside ThrowError(), let me know, though I believe it isn't strictly necessary as the core issue is fully resolved.

[FileEX]

FunctionParser still calls luaL_error, so the root cause isn't fully addressed. It looks like we're merely postponing the longjmp.

[MohabCodeX]

Calling luaL_error at the FunctionParser boundary is intentional since the stack is already unwound by then.

To address this systemically, I've updated all 86 luaL_error call sites across 10 luadefs files to use argStream.ThrowError() so every argument reader unwinds safely now.