MantisBT - Community
View Issue Details
0026913Community[OCCT] OCCT:Foundation Classespublic2015-11-22 11:152016-04-20 15:51
Roman Lygin 
bugmaster 
normalcrash 
closedfixed 
WindowsVC++ 201332 bit
[OCCT] 7.0.0 
[OCCT] 7.0.0[OCCT] 7.0.0 
0026913: Vulnerable mechanism in Standard_Type leads to assert
The Standard_Types (pointers thereto) are registered in a global repository (see GetRegistry() in Standard_Type.cxx)

Standard_Type ctor accepts char* theSystemName which is supplied as typeid(T).name() (see type_instance<T>::get ()). This theSystemName is simply stored (not copied) in Standard_Type object and is used as a key in the above registry. In Standard_Type dtor assert is triggered if this system name is not in the registry.

Apparently this approach is vulnerable as memory pointed to by typed().name() can be changed at least during library unloading. The reason why this has not been caught earlier by OCC team is likely that in all test scenarios Standard_Type objects were destroyed in the library which created them. However this is not necessarily true. The following test scenario triggered the problem:

Library A contains a static container of Handle_Standard_Type objects.

Library B which is linked to A populates the above container with types the library B created.

During application termination, B is unloaded first. Memory of its type_info’s get modified (see stack on the screenshot). However the Handle_Standard_Type objects it created still reside in the static container in library A. These Standard_Type objects (mySystemName) now point to garbage.

Then library A is unloaded and destructors of those Standard_Type objects get called. Standard_Type dctor uses mySystemName to check if the type is still in the repository and since mySystemName now points to garbage the assert is triggered.

The fix should create a copy of theSystemName in Standard_Type ctor.
Follow the steps in the description.
To catch the moment when mySystemName has changed the MS Visual Studio debugger has been used (setting breakpoint on memory change).
No tags attached.
related to 0026551closed bugmaster Open CASCADE Optimization of initialization of OCCT RTTI 
related to 0024947closed abv Open CASCADE Redesign OCCT legacy type system 
png stack.png (19,581) 2015-11-22 11:15
https://tracker.dev.opencascade.org/
Issue History
2015-11-22 11:15Roman LyginNew Issue
2015-11-22 11:15Roman LyginAssigned To => abv
2015-11-22 11:15Roman LyginFile Added: stack.png
2015-11-22 13:59abvRelationship addedrelated to 0026551
2015-11-22 14:55gitNote Added: 0048444
2015-11-22 14:56Roman LyginStatusnew => resolved
2015-11-22 14:56Roman LyginSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=12349#r12349
2015-11-22 15:08Roman LyginNote Added: 0048445
2015-11-22 19:38abvNote Added: 0048446
2015-11-22 20:01Roman LyginNote Added: 0048447
2015-11-22 20:19abvNote Added: 0048448
2015-11-22 20:39abvNote Added: 0048450
2015-11-22 21:17Roman LyginNote Added: 0048451
2015-11-23 06:51abvNote Added: 0048456
2015-11-30 06:46abvRelationship addedrelated to 0024947
2015-12-16 21:04bugmasterAssigned Toabv => bugmaster
2015-12-16 21:04bugmasterStatusresolved => reviewed
2015-12-16 21:04bugmasterNote Added: 0049220
2015-12-16 21:04bugmasterStatusreviewed => tested
2015-12-18 15:25bugmasterChangeset attached => occt master a0218ba1
2015-12-18 15:25bugmasterStatustested => verified
2015-12-18 15:25bugmasterResolutionopen => fixed
2016-04-17 14:14gitNote Added: 0053109
2016-04-20 15:42aivFixed in Version => 7.0.0
2016-04-20 15:51aivStatusverified => closed

Notes
(0048444)
git   
2015-11-22 14:55   
Branch CR26913 has been created by Roman Lygin.

SHA-1: e67fc646118e8fc942accf0a59d22865b4fd62e3


Detailed log of new commits:

Author: Roman Lygin
Date: Sun Nov 22 14:54:39 2015 +0400

    0026913: Vulnerable mechanism in Standard_Type leads to assert
(0048445)
Roman Lygin   
2015-11-22 15:08   
Copying of theSystemName obviously has a performance penalty (heap allocation + string copy) paid during library loading.

Several approaches are possible:
1. Do nothing - accept the costs. Extra allocation and copy should be comparable with other overhead used in type registry.
2. Use registry in debug mode only, ignore mySystemName otherwise.
3. Ignore mySystemName and use theName as registry keys.
4. Do not use registry altogether. Avoid using mySystemName.
(0048446)
abv   
2015-11-22 19:38   
Roman, have a look at 0026551 -- I believe this can be a way to optimize loading time.
(0048447)
Roman Lygin   
2015-11-22 20:01   
Andrey, I did and am not certain I understood the idea, sorry.
I did not catch the issue of a risk of dual initialization. Neither did I catch the avoidance of Standard_Type::myInstance. To ensure const& there must be a static variable anyway, must it not ? We can discuss how to reduce this static variable construction time (some ideas are above) but we cannot eliminate them altogether, can we ?
(0048448)
abv   
2015-11-22 20:19   
The basic idea is to avoid global static variables holding type instances, and keep only static vars inside functions get()
(0048450)
abv   
2015-11-22 20:39   
Regarding your points above, consider that registry is necessary to ensure that descriptors of the same type initialized by different libraries are the same -- each library maintains its own copy of static variable of the type, and without registry they would be different (so that you cannot use IsKin() method). Thus, variants 2 and 4 are not suitable.

Point 3 does not make any difference -- the string used in it is also statically allocated in the stack of the library that created the descriptor, thus using it would lead to the same problem as current version.
(0048451)
Roman Lygin   
2015-11-22 21:17   
Re point 3 - location of string literals is implementation-dependent but rather in static memory, and definitively should not be on stack. AFAIU, when you initialize a variable during run-time dynamic initializer there is no stack per se, except when you initialize a variable by a function (in which case the stack will be used by that function). Some comments can be found here - http://stackoverflow.com/questions/4970823/where-in-memory-are-string-literals-stack-heap. [^] BTW, on VS 2015 when mySystemName got corrupted myName was correct.

Regarding registry necessity I doubt it in a first place - as explained here (http://dev.opencascade.org/index.php?q=node/1137 [^]), initialization of static variables should rather be done out of line, inside each respective library. Essentially this existed in pre 7.0 versions, when each library was in charge of creating its own static variables. Inlining virtual DynamicType() method does not seem to bring any good but brought unnecessary complexity to deal with static variables of myInstance that can reside in multiple libraries. So why just not to revert this back and drop this registry altogether ?
(0048456)
abv   
2015-11-23 06:51   
For point 3: yes, you are completely right, I meant static area rather than stack. The important point is that this area gets loaded (and unloaded) along with the library.
(0049220)
bugmaster   
2015-12-16 21:04   
Fix has been tested with fix for issue 0026988
(0053109)
git   
2016-04-17 14:14   
Branch CR26913 has been deleted by kgv.

SHA-1: e67fc646118e8fc942accf0a59d22865b4fd62e3