Incorrect usage of StackPopString in jvm/ruby/lua plugins

[Skywing]

The various alternate script interpreter projects appear to be operating based on a common code base with a problem in its usage of CVirtualMachine::StackPopString.

Specifically, CVirtualMachine::StackPopString transfers ownership of the CExoString buffer returned to the caller. The caller must free the buffer (i.e. the string pointer within the CExoString), or they will gradually leak memory and eventually take down the server when memory is exhausted.

Additionally, it's not necessary to allocate the CExoString whose pointer is passed to StackPopString on the heap; a stack based CExoString is fine (and in fact this is what the majority of the game's action service handlers do).

Be sure to use the same memory allocator as the game when freeing the memory, or else heap corruption may occur (i.e., check out CExoString::~CExoString). For Windows builds in particular, this means that assuming that "free" is the right function is a bad idea.

[motu99]

The problem is subtle and not only pertains to the alternate script interpreter projects, but to ALL nwnx code.

The root of the problem is this:

nwserver uses its own memory allocate / deallocate routines. Lets call them

nwserver::new()
nwserver::free()

These routines have been statically linked into the nwserver code, when Bioware compiled the source with Visual C++ 6.0.

nwnx uses its own instances of free() and new(), depending on the compiler used to compile the various nwnx dlls. Lets call them

nwnx::new()
nwnx::free()

new() and free() are opposite twins. They have access to the same data-structure. When new() allocates a piece of memory, it places the relevant information in the common data-structure maintained by new/free. When free() is called on a pointer allocated by new(), the common data-structure is searched and updated.

But the pair nwnx::new() + nwnx::free() doesn't know anything about the pair nwserver::new() + nwserver::free().

Even if the code for nwserver::free() + nwserver::new() and nwnx::free() + nwnx::new() is exactly the same (which most likely is not so, because nwserver and nwnx were compiled with different compilers), the nwnx and nwserver instances don't share the same data structures: So when a piece of memory from the heap is allocated via nwserver::new(), and we then call nwnx::free() to free it, it will NOT be freed, because nwnx::free() cannot find that piece of memory in its data-structures.

Now lets look at the code of one of the most important nwnx-functions, e.g. the hook code into SetLocalString. The low-level hook code (partly coded as inline-assembly) calls the high level C-function PayLoad(). PayLoad() does all the work. I commented the problematic parts:

[virusman]

Btw, this problem doesn't exist on Linux.
Also, it seems very odd to me that no one has noticed any memleaks in all these years of extensive use.
_________________
In Soviet Russia, NWN plays you!

[Skywing]

motu's analysis is correct. This is a specific instance of the "always make sure you free memory to the same allocator you got it from" problem. In the general case it's not really safe to assume that the C runtime "free" is the correct allocator without making sure first.

For what it's worth, the NWNX4 NWNXGetString is not impacted by this problem in that the NWNXGetString implementation retains ownership of the returned string (nwn2server makes a local copy of it immediately on its own heap, and manages the lifetime itself). Of course this isn't true for NWN1 where there aren't the dedicated NWNX interface APIs.

[motu99]

The memory leak probably isn't noticeable, because the problematic part of the code (e.g. the piece of code with the free() function) is almost never executed: Most of the nwnx-interface functions (in the interface files nwnx***.nss) make sure, that they pass a long enough string to nwnx in order to cover the largest return string length. In that case nwnx copies the SMALLER return string into the char* allocated by nwserver. As long as StringLengthPassedIntoNWNX >= StringLengthReturnedToNWServer everything is fine.