b2BlockAllocator is unsafe [w/ FIX]

General Box2D issues or C++ specific issues
WalkerWhite
Posts: 1
Joined: Thu Feb 11, 2016 3:26 pm

b2BlockAllocator is unsafe [w/ FIX]

Postby WalkerWhite » Thu Feb 11, 2016 3:33 pm

The class b2BlockAllocator violates proper C++ conventions as outlined in Scott Meyer's "Effective C++". As a result, it is possible to have an EXTREMELY rare circumstance (rare enough that this may be its first discovery) in which the physics world memory becomes corrupted. Fixing b2BlockAllocator to follow proper coding guidelines removes this problem.

If you just want to see the fix, scroll down. However, it helps to understand why this fix is important (and why it is so rare). So let me give you some background.

I am using Cocos2d for a class, but am making some modifications to that engine to make it more palatable for my students. I have a love-hate relationship with Cocos2d. It is the lesser of all evils when it comes to cross-platform open-source mobile development, but it makes a lot of bad design decisions. Hence my modified version, which we will call Cocos2d++.

When you are compiling a game on Windows for either Win32 or Android, you have three distinct compilation units: the Game, Cocos2d++, and Box2d. Most of the time, when you use Box2d, your usage is entirely constrained to a single compilation unit. Cocos2d++ uses Box2d for its scene graphs, but does not allow the game direct access to this world. If you want direct access to a physics world in your game, you allocate the world and use it entirely in the Game compilation unit. None of these uses will show off the bug, which is why this might be its first discovery.

In Cocos2d++ there is a workflow where the game world is allocated in the Cocos2d++ unit, but the world is exposed to the Game unit. The Game unit is then permitted to call the methods CreateBody() and CreateJoint(). However, the bug in b2BlockAllocator does not allow b2World to safely pass across compilation units like this, and those methods will fail.

You can argue that you do not like this design. But the fact remains that rewriting b2BlockAllocator to properly follow C++ guidelines fixes this problem.

The problem is with the static arrays s_blockSizes and s_blockSizeLookup. These arrays are not on the heap and so are allocated statically. The order of initialization of non-local static objects is undefined in the case of multiple compilation units. So the Game unit has one version of the arrays s_blockSizes and s_blockSizeLookup, and Cocos2d++ has a completely different set of arrays.

When you create a world, it populates these static arrays for the first time. If the world is created in Cocos2d++, that means it is safe to allocate physics objects in Cocos2d++ only. If the world is passed to the Game unit, the static arrays are now uninitialized (even though the b2World thinks otherwise) and it is unsafe to allocate any physics objects.

The solution is to follow proper guidelines and add instance variables to b2BlockAllocator to refer to the static objects, so that the reference is preserved across compilation units.

THE FIX

Add the following private fields to the class b2BlockAllocator in b2BlockAllocator.h

Code: Select all

   int32* m_blockSizes;
   uint8* m_blockSizeLookup;


At the end of b2BlockAllocator::b2BlockAllocator() in b2BlockAllocator.cpp, add the lines

Code: Select all

   m_blockSizes = &(s_blockSizes[0]);
   m_blockSizeLookup = &(s_blockSizeLookup[0]);


Finally, replace all instances of the static variables s_blockSizes and s_blockSizeLookup in b2BlockAllocator::Allocate(int32 size) and b2BlockAllocator::Free(void* p, int32 size) with the instance versions.

These instance variables will ensure that the allocation space is consistent across multiple compilation units, allowing b2World to be passed back and forth freely.

Return to “Bugs, Requests, and Feedback”



Who is online

Users browsing this forum: No registered users and 2 guests