MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0026913Community[OCCT] OCCT:Foundation Classespublic2015-11-22 11:152016-04-20 15:51
ReporterRoman Lygin 
Assigned Tobugmaster 
PrioritynormalSeveritycrash 
StatusclosedResolutionfixed 
PlatformWindowsOSVC++ 2013OS Version32 bit
Product Version[OCCT] 7.0.0 
Target Version[OCCT] 7.0.0Fixed in Version[OCCT] 7.0.0 
Summary0026913: Vulnerable mechanism in Standard_Type leads to assert
DescriptionThe 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.
Steps To ReproduceFollow the steps in the description.
Additional information
and documentation updates
To catch the moment when mySystemName has changed the MS Visual Studio debugger has been used (setting breakpoint on memory change).
TagsNo tags attached.
Test case number
Attached Filespng file icon stack.png (19,581 bytes) 2015-11-22 11:15

- Relationships
related to 0026551closedbugmaster Open CASCADE Optimization of initialization of OCCT RTTI 
related to 0024947closedabv Open CASCADE Redesign OCCT legacy type system 

-  Notes
(0048444)
git (administrator)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (manager)
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 (developer)
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 (manager)
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 (administrator)
2015-12-16 21:04

Fix has been tested with fix for issue 0026988
(0053109)
git (administrator)
2016-04-17 14:14

Branch CR26913 has been deleted by kgv.

SHA-1: e67fc646118e8fc942accf0a59d22865b4fd62e3

- Related Changesets
occt: master a0218ba1
Timestamp: 2015-11-22 10:54:39
Author: Roman Lygin
Committer: bugmaster
Details ] Diff ]
0026913: Vulnerable mechanism in Standard_Type leads to assert
mod - src/Standard/Standard_Type.cxx Diff ] File ]
mod - src/Standard/Standard_Type.hxx Diff ] File ]

- Issue History
Date Modified Username Field Change
2015-11-22 11:15 Roman Lygin New Issue
2015-11-22 11:15 Roman Lygin Assigned To => abv
2015-11-22 11:15 Roman Lygin File Added: stack.png
2015-11-22 13:59 abv Relationship added related to 0026551
2015-11-22 14:55 git Note Added: 0048444
2015-11-22 14:56 Roman Lygin Status new => resolved
2015-11-22 14:56 Roman Lygin Steps to Reproduce Updated View Revisions
2015-11-22 15:08 Roman Lygin Note Added: 0048445
2015-11-22 19:38 abv Note Added: 0048446
2015-11-22 20:01 Roman Lygin Note Added: 0048447
2015-11-22 20:19 abv Note Added: 0048448
2015-11-22 20:39 abv Note Added: 0048450
2015-11-22 21:17 Roman Lygin Note Added: 0048451
2015-11-23 06:51 abv Note Added: 0048456
2015-11-30 06:46 abv Relationship added related to 0024947
2015-12-16 21:04 bugmaster Assigned To abv => bugmaster
2015-12-16 21:04 bugmaster Status resolved => reviewed
2015-12-16 21:04 bugmaster Note Added: 0049220
2015-12-16 21:04 bugmaster Status reviewed => tested
2015-12-18 15:25 bugmaster Changeset attached => occt master a0218ba1
2015-12-18 15:25 bugmaster Status tested => verified
2015-12-18 15:25 bugmaster Resolution open => fixed
2016-04-17 14:14 git Note Added: 0053109
2016-04-20 15:42 user533 Fixed in Version => 7.0.0
2016-04-20 15:51 user533 Status verified => closed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker