View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0021977Open CASCADEOCCT:Application Frameworkpublic2010-08-18 17:542012-11-16 13:17
ReporterabvAssigned Toszy 
PrioritynormalSeveritytrivial 
Status closedResolutionfixed 
OSAll 
Product Version6.5.1 
Target Version6.5.4Fixed in Version6.5.4 
Summary0021977: Unsafe implementation of TNaming_Builder
DescriptionClass TNaming_Builder is implemented in very unsafe way: upon creation it
creates new TNaming_Named shape attribute (which is bad on itself -- creation
of the tool should not modify the document) and remembers plain pointer to it.
The attribute can be then removed (with ForgetAttribute()), while the tool is
unable to know this; on any next call it will attempt to use this attribute as
it were alive. This can lead to crashes and unpredictable behavior.

The test case for this is chl 920 B7; see relevant code in QAAlcatel.cxx,
function OCC361bug()

This problem has been diagnosed during testing of patch on OCC21961 (v6)
TagsNo tags attached.
Test case numberchl 920 B7

Attached Files

  • OCC21977-patch-v1.zip (12,560 bytes)

Relationships

related to 0023086 closedszy Community OCC 6.5.2 DNaming bug 
parent of 0023038 closedvro Open CASCADE QA command OCC361 gives an exception 

Activities

2010-10-15 14:34

 

OCC21977-patch-v1.zip (12,560 bytes)

bugmaster

2011-07-29 14:20

administrator   ~0017859

Dear Andrey,

SVN workbench http://svn/svn/occt/branches/OCC21977
has been created and ready to be revised

abv

2011-08-22 17:12

manager   ~0018013

As far as I can see, the proposed fix does not solve the problem: class TNaming_Builder continues to hold reference to the attribute that it creates upon initialization (field myAtt), and still uses it. Regardless of the added checks for presence of this kind of attribute on the label, this myAtt field is used instead of the one actually present.

Note that test case in QAAlcatel.cxx became more complex than before, and I am sure it will fail in the same way as before if second call to Generated() is removed.

szy

2011-09-20 16:40

manager   ~0018224

1. The proposed fix solves the problem as it is not more reproduced on the specified test case (just check the returned attribute for Null). TNaming_Builder should continue to hold reference to the attribute because of a)logic of its work and b)test cases when Builder is used in very intensive way (creating sequences of NamedShapes). Forcing each time find it in the Document leads to sufficient performance problems.

2. If the test case in QAAlcatel.cxx became more complex than before the best way is provide it and check once again. The attached patch guarantee normal behavior (without exceptions) if each after extracting it from Builder a developer will check it for Null (it seems natural action when work with handles).

mkv

2011-09-30 12:16

tester   ~0018297

Dear BugMaster,
Workbench KAS:dev:mkv-OCC21977-occt was created from SVN branch http://svn/svn/occt/branches/OCC21977
(and mkv-OCC21977-products from trunk) and compiled on Linux platform.

There are not regressions in mkv-OCC21977-products regarding to KAS:dev:products-20110810-opt

See results in /dn45/occttests/results/KAS/dev/mkv-OCC21977-products_27092011/lin
See reference results in /dn45/occttests/results/KAS/dev/products-20110810-opt_11082011/lin
See test cases in /dn45/occttests/tests/ED
N.B. In order to launch testing case you can make use the following instructions
http://doc/doku.php?id=occt:certification

abv

2012-09-07 09:57

manager   ~0021429

The fix is integrated in branch CR21977, please review.

Note that behavior of this class is not changed; I have only replaced plain C pointers in class fields to Handles, to prevent possible use of freed memory. Notably this fix immediately caused failure of multiple tests due to bug described in 0023086. Hence this fix is put on top of the branch CR23086, and should be tested and integrated with that.

szy

2012-09-07 15:58

manager   ~0021434

Reviewed.

mkv

2012-09-13 15:07

tester   ~0021479

Last edited: 2012-09-13 15:08

 Dear BugMaster,
Branch CR21977 (and products from GIT master) was compiled on Linux and Windows platforms and tested.

Regression:
Not detected

Improvements:
Not detected

Testing case:
chl 920 B7

Related Changesets

occt: master 8b8bffc6

2012-09-14 13:20:57

szy

Details Diff		 0021977: Unsafe implementation of TNaming_Builder

The code is corrected to create instances of TNaming_Builder class dynamically. Note that they cannot be created as local variables as they should be instantiated only when needed and then reused for the subshapes of the same type in cycle.

Code around is cleaned from tabs and duplicated fragments.

TNaming_Builder class is changed to use Handles instead of C pointers in its fields.
This should protect from possible access to the freed memory if attribute is deleted while instance of TNaming_Builder is still alive.
In addition, method to construct dummy vertex for storing orientation is simplified.		 Affected Issues
0021977
mod - src/TNaming/TNaming_Builder.cdl Diff File
mod - src/TNaming/TNaming_NamedShape.cxx Diff File

Issue History

Date Modified Username Field Change
2010-09-06 12:08 bugmaster Assigned To bugmaster => szy
2010-09-06 12:08 bugmaster Status new => assigned
2010-10-12 19:46 szy CC => abv
2010-10-15 16:34 szy Status assigned => resolved
2011-07-29 14:20 bugmaster Note Added: 0017859
2011-07-29 14:20 bugmaster Assigned To szy => abv
2011-07-29 14:20 bugmaster Status resolved => assigned
2011-07-29 14:20 bugmaster Status assigned => resolved
2011-07-29 14:20 bugmaster Fixed in Version EMPTY =>
2011-07-29 14:20 bugmaster Description Updated
2011-08-02 10:32 bugmaster Category OCCT:OCAF => OCCT:Application Framework
2011-08-09 12:34 bugmaster Test case number => chl 920 B7
2011-08-22 17:12 abv Note Added: 0018013
2011-08-22 17:12 abv Assigned To abv => szy
2011-08-22 17:12 abv Status resolved => assigned
2011-09-20 16:40 szy Note Added: 0018224
2011-09-20 16:40 szy Assigned To szy => bugmaster
2011-09-20 16:40 szy Status assigned => reviewed
2011-09-30 12:16 mkv Note Added: 0018297
2011-09-30 12:16 mkv Status reviewed => tested
2011-09-30 18:49 abv Assigned To bugmaster => abv
2011-09-30 18:49 abv Status tested => assigned
2012-03-27 12:43 abv Relationship added parent of 0023038
2012-09-07 09:22 abv Summary Perverse implementation of TNaming_Builder => Unsafe implementation of TNaming_Builder
2012-09-07 09:22 abv Description Updated
2012-09-07 09:34 abv Relationship added related to 0023086
2012-09-07 09:57 abv Note Added: 0021429
2012-09-07 09:57 abv Assigned To abv => szy
2012-09-07 09:57 abv Status assigned => resolved
2012-09-07 15:58 szy Note Added: 0021434
2012-09-07 15:58 szy Assigned To szy => mkv
2012-09-07 15:58 szy Status resolved => reviewed
2012-09-10 16:28 szy Product Version => 6.5.1
2012-09-10 16:28 szy Target Version => 6.5.4
2012-09-13 15:07 mkv Note Added: 0021479
2012-09-13 15:08 mkv Note Edited: 0021479
2012-09-13 15:10 mkv Assigned To mkv => bugmaster
2012-09-13 15:10 mkv Status reviewed => tested
2012-09-17 17:29 szy Changeset attached => occt master 8b8bffc6
2012-09-17 17:33 szy Assigned To bugmaster => szy
2012-09-17 17:33 szy Status tested => verified
2012-11-16 13:14 bugmaster Fixed in Version => 6.5.4
2012-11-16 13:17 bugmaster Status verified => closed