logo logo

 Back to main page

The NWNX Community Forum

 FAQFAQ   SearchSearch   MemberlistMemberlist   UsergroupsUsergroups   RegisterRegister 
 ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 
 
Incorrect usage of StackPopString in jvm/ruby/lua plugins

 
Post new topic   Reply to topic    nwnx.org Forum Index -> Linux development
View previous topic :: View next topic  
Author Message
Skywing



Joined: 03 Jan 2008
Posts: 321

PostPosted: Mon Jun 06, 2011 7:49    Post subject: Incorrect usage of StackPopString in jvm/ruby/lua plugins Reply with quote

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.
Back to top
View user's profile Send private message
motu99



Joined: 24 Sep 2007
Posts: 16

PostPosted: Wed Jun 08, 2011 20:28    Post subject: Reply with quote

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:

Code:

void PayLoad(char *gameObject, char* name, char** ppvalue)
158   {

              [...]

196                   // library found, handle the request
197                   iValueLength = strlen(value); // WARNING: char* value was allocated with nwserver::new()
198                   char* pRes = pBase->OnRequest(gameObject, function + 1, value); // WARNING: char* pRes is (in most cases) allocated by nwnx::new()
199                   if (pRes != NULL)
200                   {
201                           if(strncmp(library,"LETO",4) != 0 &&
202                              strncmp(library,"HASHSET",7) != 0)
203                           {
204                                   // copy result into nwn variable value while respecting the maximum size
205                                   // new plugins
206                                   iResultLength = strlen(pRes);
207                                   if (iValueLength < iResultLength)
208                                   {
209                                           free(value); // MEMORY LEAK: using nwnx::free() on value, which was allocated with nwserver::new()
210                                           *ppvalue = pRes; // WARNING: passing pRes (allocated with nwnx::new()) back to nwserver, which will eventually try to free it with nwserver::free(), resulting in another memory leak
211                                           *((unsigned long *)ppvalue+1) = strlen(pRes);
212                                   }
213                                   else
214                                   {
215                                           strncpy(value, pRes, iResultLength); // NO PROBLEM
216                                           *(value+iResultLength) = 0x0;
217                                           free(pRes);
218                                   }
219                           }
220                           else
221                           {
222                                   // copy result into nwn variable value while respecting the maximum size
223                                   // legacy plugins
224                                   iResultLength = strlen(pRes);
225                                   if (iValueLength < iResultLength)
226                                   {
227                                           strncpy(value, pRes, iValueLength); // NO PROBLEM
228                                           *(value+iValueLength) = 0x0;
229                                   }
230                                   else
231                                   {
232                                           strncpy(value, pRes, iResultLength); // NO PROBLEM
233                                           *(value+iResultLength) = 0x0;
234                                   }
235   
236                           }
237                   }


The remedy is:

always use the correct new() and free() instances.
When passing a pointer back to nwserver, make sure its memory was allocated with nwserver::new()
When freeing a pointer, make sure it is freed with the correct instance of free(): e.g. if the pointer was allocated by nwserver, use nwserver::free(), otherwise use nwnx::free()

EDIT: Problems might also arise for different dlls that pass pointers to and fro. If every dll is statically linked to its own new/free pair there might be memory leaks as well. I don't know enough about how dlls work to be sure, though.
Back to top
View user's profile Send private message
virusman



Joined: 30 Jan 2005
Posts: 1020
Location: Russia

PostPosted: Wed Jun 08, 2011 21:54    Post subject: Reply with quote

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!
Back to top
View user's profile Send private message Visit poster's website Yahoo Messenger
Skywing



Joined: 03 Jan 2008
Posts: 321

PostPosted: Wed Jun 08, 2011 22:34    Post subject: Reply with quote

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.
Back to top
View user's profile Send private message
motu99



Joined: 24 Sep 2007
Posts: 16

PostPosted: Wed Jun 08, 2011 23:11    Post subject: Reply with quote

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.
Back to top
View user's profile Send private message
Display posts from previous:   
Post new topic   Reply to topic    nwnx.org Forum Index -> Linux development All times are GMT + 2 Hours
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum


Powered by phpBB © 2001, 2005 phpBB Group