View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0026913 | Community | OCCT:Foundation Classes | public | 2015-11-22 11:15 | 2016-04-20 15:51 |
| Reporter | Roman Lygin | Assigned To | bugmaster | ||
| Priority | normal | Severity | crash | ||
| Status | closed | Resolution | fixed | ||
| Platform | Windows | OS | VC++ 2013 | ||
| Product Version | 7.0.0 | ||||
| Target Version | 7.0.0 | Fixed in Version | 7.0.0 | ||
| Summary | 0026913: Vulnerable mechanism in Standard_Type leads to assert | ||||
| Description | 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. | ||||
| Steps To Reproduce | Follow 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). | ||||
| Tags | No tags attached. | ||||
| Test case number | |||||
|
|
stack.png (19,581 bytes) |
|
|
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 |
|
|
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. |
|
|
Roman, have a look at 0026551 -- I believe this can be a way to optimize loading time. |
|
|
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 ? |
|
|
The basic idea is to avoid global static variables holding type instances, and keep only static vars inside functions get() |
|
|
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. |
|
|
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 ? |
|
|
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. |
|
|
Fix has been tested with fix for issue 0026988 |
|
|
Branch CR26913 has been deleted by kgv. SHA-1: e67fc646118e8fc942accf0a59d22865b4fd62e3 |
| 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 |
|
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 | |
| 2015-11-22 15:08 | Roman Lygin | Note Added: 0048445 | |
| 2015-11-22 19:38 |
|
Note Added: 0048446 | |
| 2015-11-22 20:01 | Roman Lygin | Note Added: 0048447 | |
| 2015-11-22 20:19 |
|
Note Added: 0048448 | |
| 2015-11-22 20:39 |
|
Note Added: 0048450 | |
| 2015-11-22 21:17 | Roman Lygin | Note Added: 0048451 | |
| 2015-11-23 06:51 |
|
Note Added: 0048456 | |
| 2015-11-30 06:46 |
|
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 |
|
Fixed in Version | => 7.0.0 |
| 2016-04-20 15:51 |
|
Status | verified => closed |
