Archived issue #0021977
Unsafe implementation of TNaming_Builder
Description
Class 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)
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)
Public activity
7 archived notes
Participants are labeled by their role within this record.
Dear Andrey,
SVN workbench http://svn/svn/occt/branches/OCC21977
has been created and ready to be revised
SVN workbench http://svn/svn/occt/branches/OCC21977
has been created and ready to be revised
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.
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.
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).
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).
Dear Commenter 2,
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
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
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.
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.
Reviewed.
Dear Commenter 2,
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
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 records