search

lianlab / gamekit

Automatically exported from code.google.com/p/gamekit
0 stars 0 forks source link

AppCppDemo crash when leaving #142

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
On a Debian/Squeeze.

I have these backtrack when living AppCppDemo.

CppDemo: enter main loop.
CppDemo: exit main loop.
*** glibc detected *** ./AppCppDemo: double free or corruption (fasttop): 
0x0000000003e6db70 ***
======= Backtrace: =========
/lib/libc.so.6(+0x71ad6)[0x7ffb5157ead6]
/lib/libc.so.6(cfree+0x6c)[0x7ffb5158384c]
/usr/lib/libstdc++.so.6(_ZNSsD1Ev+0x39)[0x7ffb51daeee9]
./AppCppDemo(_ZN14utHashedStringD1Ev+0x18)[0xaf702a]
./AppCppDemo(_ZN11utHashEntryI14utHashedStringP12gkGameObjectED1Ev+0x18)[0xb27f5
4]
./AppCppDemo(_ZN11utHashTableI14utHashedStringP12gkGameObjectE5clearEb+0xa8)[0xb
27ffe]
./AppCppDemo(_ZN7gkSceneD0Ev+0x15a)[0xb22276]
./AppCppDemo(_ZN17gkResourceManager10destroyAllEv+0xad)[0xb1ebfb]
./AppCppDemo(_ZN8gkEngine8finalizeEv+0x42)[0xb00a0a]
./AppCppDemo(main+0x27b)[0xaf147f]
/lib/libc.so.6(__libc_start_main+0xfd)[0x7ffb5152bc4d]
./AppCppDemo[0xaf1149]
======= Memory map: ========
... if you need ...

Original issue reported on code.google.com by philippe...@gmail.com on 24 Feb 2011 at 8:38

GoogleCodeExporter commented 9 years ago 
Same on Ubuntu Maverick 64 bit.

Original comment by kungfoobar@gmail.com on 25 Feb 2011 at 2:31

GoogleCodeExporter commented 9 years ago 
a small patch for a delete on a local var

Original comment by philippe...@gmail.com on 3 Mar 2011 at 7:15

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks for your patch.
But the engine don't delete the game(gkGameLevel) object on finalize step.

If the game object deletion is caused error, m_player or m_pickup deletion can 
be problem.

I can't execution test on Linux.
Can you test this code? 

gkGameLevel::~gkGameLevel()
{
//comment out delete m_player or pickup, or both.

    //delete m_player;
    //m_player = 0;

.... 
    //if (m_pickup)           
           //gkSceneManager::getSingleton().destroy(m_pickup->getResourceHandle());
}

Original comment by harkon...@gmail.com on 3 Mar 2011 at 3:28

GoogleCodeExporter commented 9 years ago 
From what I understand: m_player should be deleted, it is not managed by any 
resource manager. Destroying the m_pickup scene by hand is not necessary but 
should not cause problem. I tested to confirm and indeed the crash still occur.

The problem is that when an object "MomoPhysics" in this case is added to more 
than one scene (it is added to the scene it belong by the blender loader than 
copied in the level scene). Then at exit, each scene delete its hash table of 
objects (gkScene.cpp line 132), and this frees the gkGameObject resource name 
string (not the hash or the group name). After the first scene that uses the 
object is destroyed, the object has no name anymore. Then when a second scene 
containing the object is destroyed it crashes because of a double free (it 
tries to free the game object resource name a second time).

Original comment by xavier.thomas.1980@gmail.com on 11 Mar 2011 at 5:31

GoogleCodeExporter commented 9 years ago 
The following small patch works around the issue but why does the string does 
not get deep copied without this modification? Also the demo still crash later 
in the exit procedure because an object want to free a mesh already freed. The 
cause is certainly the same and still should be found.

Note that even if this bug only happen in the CppDemo for the moment, 
potentially every game can be affected (the issue is in gamekit, not the demo 
itself).

I think this is a pretty serious bug.

Index: Engine/gkScene.cpp
===================================================================
--- Engine/gkScene.cpp  (revisão 857)
+++ Engine/gkScene.cpp  (cópia de trabalho)
@@ -393,9 +393,10 @@
 void gkScene::addObject(gkGameObject* gobj)
 {
    GK_ASSERT(gobj);
-
-   const gkHashedString name = gobj->getName();
-
+   
+   utString* sn = new utString(gobj->getName());
+   const gkHashedString name = *sn;
+   
    if (m_objects.find(name) != GK_NPOS)
    {
        gkPrintf("Scene: Duplicate object '%s' found in this scene\n", name.str().c_str());

Original comment by xavier.thomas.1980@gmail.com on 12 Mar 2011 at 4:52

GoogleCodeExporter commented 9 years ago 
Which object owns the resource string?

Original comment by bsd...@gmail.com on 12 Mar 2011 at 5:55

GoogleCodeExporter commented 9 years ago 
gobj should own the memory

Original comment by xavier.thomas.1980@gmail.com on 12 Mar 2011 at 1:10

GoogleCodeExporter commented 9 years ago 
+   utString* sn = new utString(gobj->getName());
+   const gkHashedString name = *sn;

Is it caused a memory leak?

Original comment by harkon...@gmail.com on 14 Mar 2011 at 7:30

GoogleCodeExporter commented 9 years ago 
No it does not cause a memory leak, at the contrary it avoid a double free 
crash. I will try to make a test case demonstrating/isolating the issue the 
issue.

Here is what I got until so far:
-The crash is because the gkobject resource name is freed twice.
-Using valgrind, I saw the backtrace at the time ot the first free is exactly 
the same as at the time of the crash: line 132 of gkScene
m_objects.clear(); line 132
called by gkEngine::finalize()
-Explorating the variables states after the crash showed me that when clearing 
the hashtable m_objects, one of the entry key (a gkHashedString) had his char 
array memory freed already. The hash was 36145500821
-I set a conditional breakpoint in gkScene::AddObject() with the condition 
name.m_hash==36145500821, explorating the memory of the gkObject resource name 
and the hashtables entry showed me that the 3 strings (the resource name of 
gkobjects and the hashtable entry of the 2 scenes it belongs to) share a 
pointer to the same memory containing "MomoPhysics" and was not deep copied.

Original comment by xavier.thomas.1980@gmail.com on 14 Mar 2011 at 8:14

GoogleCodeExporter commented 9 years ago 
I think, if the added object is freed twice, we should deep copy the object 
itself, not the name member only.

Original comment by harkon...@gmail.com on 15 Mar 2011 at 3:54

GoogleCodeExporter commented 9 years ago 
What about using reference counted game objects?

Original comment by bsd...@gmail.com on 17 Mar 2011 at 1:23

GoogleCodeExporter commented 9 years ago 
It is not the gameobject that got freed twice, just his name (the char array), 
not even his group name or the name hash. I will try to make the most simple 
app that I can and reproduce the bug but I don't have much time right now.

Original comment by xavier.thomas.1980@gmail.com on 17 Mar 2011 at 6:42

GoogleCodeExporter commented 9 years ago 
My guess with this is that utHashTable in in utTypes.h wich is not aware of 
utString.h so when using a string or an hashed string as an entry for a 
hashtable the default = operator is used instead of the std::string operator. 
Causing the pointer to the string to be copied but not the string itself to be 
deep copied.

Also in gkScene::AddObject(), this is where we add the hashtable entry without 
deep copying the string, if i try to force a copy using something like:

utString tmpstr;
obj->getName().copy(tmpstr);

I get an error because the type returned by obj->getName() is not the same as 
the argument inside copy() but both are supposed to be utString (typedef 
std::String utString). Because of the template coding, the error message is 
really not that useful.

Another thing to take into consideration is why the crash just happen in 
CppDemo. If the string does not get deep copied when added to a scene. Then it 
is freed when clearing the scene, then we should have a double free error when 
freeing the object (and thus its name for a second time). This with all objects 
even when they are part of one scene only.

Original comment by xavier.thomas.1980@gmail.com on 17 Mar 2011 at 7:00

GoogleCodeExporter commented 9 years ago 
I noticed the utHashTable::remove function does 2 "unorthodox" calls to the 
Entry destructor with this format:
m_bptr[m_size].~Entry();

If I'm not mistaken calling delete [] m_bptr will also call these destructors, 
which in turn will call destructors for std::string twice and thus generate the 
double free.

I'm not sure why this doesn't happen in the other samples, it might be because 
the remove method is never called in those demos?

In any case, commenting the 2 calls to ~Entry() seems to fix the problem (no 
segfault).

Valgrind doesn't list any memory leaks in these cases, although I'm not sure 
it's working properly and I think it might simply be choking on the application 
demanding CPU usage (I get less than 1 FPS when running with valgrind).

Original comment by vek...@gmail.com on 7 Jan 2012 at 5:19

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Ok further testing proved to be even more significant. Not only commenting the 
2 lines as I mentioned above fixes this issue but it also seems to fix all 
other segfaults-on-exit problems reported in the issue list (even one I 
reported myself).

For that reason I decided to create a simple patch so that less experienced 
people can apply it. You can find it in my report at issue #207 or 
alternatively download it directly from here:
http://gamekit.googlecode.com/issues/attachment?aid=2070001000&name=DoubleFree_f
ix.patch&token=Wf5mxmOJCtne0S_q3KUaBV2ErxI%3A1325975458666

Original comment by vek...@gmail.com on 7 Jan 2012 at 10:36

GoogleCodeExporter commented 9 years ago 
vekoon, thanks for tracking this out.
All this make sense, but I must admit I never messed much with C++ templates. I 
will investigate this further and double check if the patch does not introduce 
any memory leaks.

Original comment by xavier.thomas.1980@gmail.com on 22 Jan 2012 at 2:17

GoogleCodeExporter commented 9 years ago 
Fixed in r1013

Original comment by xavier.thomas.1980@gmail.com on 22 Jan 2012 at 4:59

GoogleCodeExporter commented 9 years ago 
I want to make a keyboard response and create a new Momo ,what should I do?

Original comment by zuo.ri.x...@gmail.com on 8 Apr 2012 at 12:27