View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0027104CommunityOCCT:Foundation Classespublic2016-01-21 16:282016-04-20 15:50
ReporterRoman Lygin Assigned Toabv 
PrioritynormalSeveritymajor 
Status closedResolutionfixed 
Product Version7.0.0 
Target Version7.0.0Fixed in Version7.0.0 
Summary0027104: DownCast() cannot return null for mismatched handle
DescriptionDowncast() reimplemented in 0024023 apparently exposes the issue discussed in 0026377 and can no longer return a null result for a mismatched handle.

This makes it impossible to catch/process such issues and thus undermines code that used to be robust against them.

Pseudo-code:
class Base;
class Derived;
class Derived1; //inherits Derived
class Derived2; //inherits Derived
void foo (Handle(Derived)& theValue)
{
  Handle_Base b = ...;
  Handle_Derived d = Handle_Derived::Downcast (b);
  return d;
}

Handle_Derived2 d2;
foo (d2); //in fact, in reality it can be Handle_Derived1

Handle_Derived2 dd2 = Handle_Derived2::DownCast (d2);

dd2 is null in 6.x and not null in 7.0, no way to check if it is of Derived2 type.


The regression was caught during regression tests on a real-world IGES file when running on 7.0 beta. The code could correctly catch and process the issue in 6.x and fails on 7.0.

Bug severity is set to major as this is a data correctness issue with severe consequences. It definitively deserves to be fixed before rolling out 7.0 with new handles.
Steps To ReproduceSee the attached C++ reproducer which is a synthetic reproducer of a case coming from a real-world IGES file.
TagsNo tags attached.
Test case number

Attached Files

  • main.cxx (2,358 bytes)

Relationships

related to 0024023 closedabv Open CASCADE Revamp the OCCT Handle 
related to 0026377 closedabv Open CASCADE Passing Handle objects as arguments to functions as non-const reference to base type is dangerous 
related to 0027111 closedabv Community Add generalized copy constructor in handle class for old compilers 

Activities

Roman Lygin

2016-01-21 16:28

developer  

main.cxx (2,358 bytes)

abv

2016-01-22 15:52

manager   ~0050064

Hello Roman,

The sample your provided in description is incomplete and can hardly be understood.
The sample code you attached in main.cxx is clearly incorrect.
Let's consider equivalent but shorter example:

  Handle(Geom_Circle) aCircle;
  Handle(Geom_Curve)& aCurve = aCircle;
  aCurve = new Geom_Line (gp::Origin(), gp::DZ()); // root cause
  Handle(Geom_Circle) aCircle2 = Handle(Geom_Circle)::DownCast (aCircle);
  
  if (! aCircle2.IsNull())
    std::cout << "Error: aCircle2 is non-null, but actually a line!" << std::endl;

  if (aCircle2->IsKind (STANDARD_TYPE(Geom_Line)))
    std::cout << "Error: aCircle2 is a line!" << std::endl;

Here we do very wrong things, storing pointer to Geom_Line object into handle to Geom_Curve. This breaks consistency of the code, and is a root cause of consequent problems.

You should not expect correct behavior from ill-formed code.

In your case you get field myShell of the class IGESSolid incorrect: it points to the object of incompatible type (due to 0026377). This is the root of your problem.

Then you dynamically cast that field (whose type is Handle_IGESShell) to the same type (Handle_IGESShell), and expect it to get nullified. This expectation has no grounds.

It worked in the past when DynamiCast() was accepting reference to the base type Handle(Standard_Transient) and always checked the type of the object; this way it produced NULL if the type is wrong.

In OCCT 7, DynamicCast() is a template function that accepts argument type as-is, and it uses C++ dynamic_cast<> to do the job. When you cast handle to the same type, compiler recognizes that these types are inherently compatible, and does simple assignment without any check. This is quite correct, in my opinion.

Besides, you can obtain the same behavior as before if you change the type returned by method OuterShell() to Handle_IGESEntity.

My conclusion is that current implementation of Handles in OCCT 7.0 is correct and does what it is intended for. Thus I see nothing to be fixed, but will be glad to know your opinion.

To resolve your case, I hope you will fix the root cause on your side (or in OCCT if it is there).

One possible improvement that could be done in this regard is to restrict method DownCast() to be available only for compatible types. In this case compiler would complain if you use DownCast() to convert the same or unrelated types.

Roman Lygin

2016-01-22 16:58

developer   ~0050071

Hello Andrey,

As mentioned, the reproducer is a synthetic one of what happens in real life. Let me explain.

There is an IGES file (a real-world one, arriving from a customer) where the MSBO (type 186) with D8651 refers in its field dedicated for OuterShell to D8651 (i.e. itself) instead of another entity of type Shell (type 514). This is certainly wrong and is an error of a sending system.

Now what happens during parsing and how can we protect against this ? We are talking here of OCC classes, that's why I referred to IGESSolid_* classes in the reproducer (main.cxx).

IGESSolid_ManifoldSolid has a field Handle_IGESSolid_Shell theShell (not Handle_IGESData_IGESEntity).

The parser invokes IGESSolid_ToolManifoldSolid::ReadOwnParams() which invokes
PR.ReadEntity(IR, PR.Current(), aStatus, shell) where shell is Handle_IGESSolid_Shell. IGESData_ParamReader::ReadEntity() has the last parameter of type Handle_IGESData_IGESEntity& and resolves references from a central file model using integer indices.

So, inside IGESData_ParamReader::ReadEntity() D8651 is successfully resolved to Handle_IGESSolid_ManifoldSolidBRep. However, inside IGESSolid_ToolManifoldSolid::ReadOwnParams() this last parameter shell is actually Handle_IGESData_Shell. This shell gets fed into IGESSolid_ManifoldSolid::Init() and we receive a solid referring to itself.

This works equally in 6.x and 7.0. Now how to protect against this ?

Option 1. Rewrite all _Tool* ReadOwnParams() to invoke ReadEntity() with H_IGESData_IGESEntity and then DownCast() to expected type when calling Init().
That would be a correct way to go, however would require massive rewrite and can be error-prone until such cases are detected in compile time.

Option 2. To have at least a possibility for a protection in user code to rely on DownCast(). This used to work in 6.x:
Handle_IGESSolid_Shell aShell = Handle_IGESSolid_Shell::DownCast (aSolid->OuterShell()); //will return null if it is actually not _Shell

This now does not work in 7.0 and that is a point of this bug report.

We may argue about current expected behavior of DownCast() but I think the final objective would be to prohibit such cases in compile time, similar to raw pointers or shared pointers:

class Base {};
class Derived1 : public Base {};
class Derived2 : public Base {};

void foo (Base*& b);
Derived1 d1;
foo (&d1); //compile-time error

Fortunately, I think we can work-around changed 7.0 behavior on our side using something like:
const Handle_Standard_Transient& tmp = aSolid->OuterShell();
Handle_IGESData_IGESShell aShell = Handle_IGESData_IGESShell::DownCast (tmp); //should work in both 6.x and 7.0

if (!aShell.IsNull()) {

The challenge is that there can be multiple places like this and they can be triggered anywhere (due to poor input data), being not covered by regression tests. So it can be an expected surprise later on.

Anyway, please judge yourself whether this is a 7.0 showstopper or not.

Hope this explains the situation. Please let me know if more details are required.

abv

2016-01-22 19:25

manager   ~0050079

Hello Roman,

I believe the right way is to fix 0026377. You can have a look at the prototype solution in branch CR26377. Note that I did not tried to push this to OCCT 7.0 because it would cause need for changes in quite many places.

As a work-around, you can do the same in existing code: check type of the result directly (using IsKind()).

git

2016-01-25 10:18

administrator   ~0050097

Branch CR27104 has been created by abv.

SHA-1: 441358d586e0e5abf1e5718c879735d821c0ebf8


Detailed log of new commits:

Author: abv
Date: Mon Jan 25 10:14:20 2016 +0300

    0027104: DownCast() cannot return null for mismatched handle
    
    Method DownCast() is amended to be available only when argument is actually a pointer or handle to a base class.
    When DownCast() is used for the same type, derived, or unrelated class (i.e. where there is no actual down casting), compiler error will be generated.
    
    OCCT code is updated to remove such trivial DownCast()s; a few places where DownCast() was used with argument of unrelated type are corrected.
    
    DRAW command QAHandleCast is removed (it was useful only during redesign of handles).

abv

2016-01-25 10:25

manager   ~0050098

Hello Roman, have a look at the change in branch CR27104 -- it forbids DownCast() for argument which is not a base type, to prevent using it where it will never work (if classes are unrelated) or does nothing (e.g. casting handle to the same type).

Roman Lygin

2016-01-25 15:57

developer   ~0050102

Hello Andrey,
Thank for the patch. Yes, it looks reasonable and might help mitigate the issue.

We also looked at 0026377 branch (though it's currently not-rebased and does not have compiler-specific SFINAE-based code). Seems to work indeed, at least in run-time (as compile-time seems possible).

Anyway, we will try to clean up our own code to get rid of a need of explicit SafeUpCast() call.

kgv

2016-02-16 11:40

developer   ~0050799

Related modification within CR27111_6 has been reviewed without remarks.
Should be tested in scope of 0027111.

bugmaster

2016-02-19 10:38

administrator   ~0050991

Tested in framework of branch CR27111_6

git

2016-04-17 13:18

administrator   ~0052838

Branch CR27104 has been deleted by kgv.

SHA-1: 441358d586e0e5abf1e5718c879735d821c0ebf8

Related Changesets

occt: master a9dde4a3

2016-01-25 07:14:20

abv


Committer: abv Details Diff		 0027104: DownCast() cannot return null for mismatched handle

Method DownCast() is made template, to be available only when argument is actually a pointer or handle to a base class.

For compatibility with existing code, method DownCast() that can be used for the same type, derived, or unrelated class (i.e. where there is no actual down casting) is still available, its use shall cause "deprecated" compiler warning.

OCCT code is updated to remove meaningless DownCast()s; a few places where DownCast() was used with argument of unrelated type are corrected.

DRAW command QAHandleCast is removed (it was useful only during redesign of handles).		 Affected Issues
0027104
mod - dox/dev_guides/upgrade/upgrade.md Diff File
mod - src/AIS/AIS_AttributeFilter.cxx Diff File
mod - src/AIS/AIS_InteractiveContext_1.cxx Diff File
mod - src/AIS/AIS_LocalContext.cxx Diff File
mod - src/BinTObjDrivers/BinTObjDrivers_ReferenceDriver.cxx Diff File
mod - src/BRepAlgo/BRepAlgo_NormalProjection.cxx Diff File
mod - src/BRepFeat/BRepFeat_MakeLinearForm.cxx Diff File
mod - src/BRepFeat/BRepFeat_MakeRevolutionForm.cxx Diff File
mod - src/BRepFeat/BRepFeat_RibSlot.cxx Diff File
mod - src/BRepFill/BRepFill_ACRLaw.cxx Diff File
mod - src/BRepFill/BRepFill_Evolved.cxx Diff File
mod - src/ChFi2d/ChFi2d_Builder.cxx Diff File
mod - src/ChFi2d/ChFi2d_Builder_0.cxx Diff File
mod - src/DDocStd/DDocStd.cxx Diff File
mod - src/DDocStd/DDocStd_DrawDocument.cxx Diff File
mod - src/DrawDim/DrawDim_PlanarDistance.cxx Diff File
mod - src/Geom/Geom_OffsetSurface.cxx Diff File
mod - src/Geom/Geom_RectangularTrimmedSurface.cxx Diff File
mod - src/Geom2dConvert/Geom2dConvert.cxx Diff File
mod - src/GeomAdaptor/GeomAdaptor_Surface.cxx Diff File
mod - src/GeomConvert/GeomConvert.cxx Diff File
mod - src/GeomliteTest/GeomliteTest_CurveCommands.cxx Diff File
mod - src/GeomPlate/GeomPlate_BuildPlateSurface.cxx Diff File
mod - src/IGESAppli/IGESAppli_Node.cxx Diff File
mod - src/IGESControl/IGESControl_IGESBoundary.cxx Diff File
mod - src/IGESSelect/IGESSelect.cxx Diff File
mod - src/IGESToBRep/IGESToBRep_IGESBoundary.cxx Diff File
mod - src/IntPatch/IntPatch_Intersection.cxx Diff File
mod - src/IVtkDraw/IVtkDraw_HighlightAndSelectionPipeline.cxx Diff File
mod - src/MAT2d/MAT2d_Tool2d.cxx Diff File
mod - src/Message/Message_Messenger.cxx Diff File
mod - src/QABugs/QABugs_PresentableObject.cxx Diff File
mod - src/QANCollection/QANCollection_Handle.cxx Diff File
mod - src/RWStepGeom/RWStepGeom_RWBSplineCurveWithKnotsAndRationalBSplineCurve.cxx Diff File
mod - src/RWStepGeom/RWStepGeom_RWBSplineSurfaceWithKnotsAndRationalBSplineSurface.cxx Diff File
mod - src/SelectMgr/SelectMgr_SelectionManager.cxx Diff File
mod - src/ShapeBuild/ShapeBuild_Edge.cxx Diff File
mod - src/ShapeConstruct/ShapeConstruct.cxx Diff File
mod - src/ShapeCustom/ShapeCustom_BSplineRestriction.cxx Diff File
mod - src/ShapeFix/ShapeFix_Face.cxx Diff File
mod - src/ShapeFix/ShapeFix_Shape.cxx Diff File
mod - src/ShapeFix/ShapeFix_Wire.cxx Diff File
mod - src/ShapeProcess/ShapeProcess_OperLibrary.cxx Diff File
mod - src/ShapeUpgrade/ShapeUpgrade_ConvertCurve2dToBezier.cxx Diff File
mod - src/ShapeUpgrade/ShapeUpgrade_ConvertCurve3dToBezier.cxx Diff File
mod - src/ShapeUpgrade/ShapeUpgrade_SplitSurface.cxx Diff File
mod - src/Standard/Standard_Handle.hxx Diff File
mod - src/StdObjMgt/StdObjMgt_ReadData.hxx Diff File
mod - src/StdSelect/StdSelect_ViewerSelector3d.cxx Diff File
mod - src/StepAP209/StepAP209_Construct.cxx Diff File
mod - src/STEPCAFControl/STEPCAFControl_Reader.cxx Diff File
mod - src/STEPCAFControl/STEPCAFControl_Writer.cxx Diff File
mod - src/STEPSelections/STEPSelections_Counter.cxx Diff File
mod - src/STEPSelections/STEPSelections_SelectInstances.cxx Diff File
mod - src/StepToGeom/StepToGeom.cxx Diff File
mod - src/StepToGeom/StepToGeom_MakeBSplineCurve.pxx Diff File
mod - src/TCollection/TCollection_HExtendedString.cxx Diff File
mod - src/TestTopOpe/TestTopOpe_HDSCommands.cxx Diff File
mod - src/TestTopOpeDraw/TestTopOpeDraw_Displayer.cxx Diff File
mod - src/TObj/TObj_Object.cxx Diff File
mod - src/TObj/TObj_TReference.cxx Diff File
mod - src/ViewerTest/ViewerTest.cxx Diff File
mod - src/VrmlData/VrmlData_Scene.cxx Diff File
mod - src/XmlTObjDrivers/XmlTObjDrivers_ReferenceDriver.cxx Diff File
mod - src/XSDRAWSTLVRML/XSDRAWSTLVRML.cxx Diff File

Issue History

Date Modified Username Field Change
2016-01-21 16:28 Roman Lygin New Issue
2016-01-21 16:28 Roman Lygin Assigned To => abv
2016-01-21 16:28 Roman Lygin File Added: main.cxx
2016-01-21 16:30 Roman Lygin Relationship added related to 0024023
2016-01-21 16:31 Roman Lygin Relationship added related to 0026377
2016-01-21 17:51 abv Target Version 7.1.0 => 7.0.0
2016-01-22 15:52 abv Note Added: 0050064
2016-01-22 15:52 abv Assigned To abv => Roman Lygin
2016-01-22 15:52 abv Status new => feedback
2016-01-22 16:58 Roman Lygin Note Added: 0050071
2016-01-22 16:58 Roman Lygin Assigned To Roman Lygin => abv
2016-01-22 16:58 Roman Lygin Status feedback => assigned
2016-01-22 19:25 abv Note Added: 0050079
2016-01-25 10:09 abv Summary [Regression in 7.0 beta] DownCast() cannot return null for mismatched handle => DownCast() cannot return null for mismatched handle
2016-01-25 10:18 git Note Added: 0050097
2016-01-25 10:25 abv Note Added: 0050098
2016-01-25 10:25 abv Assigned To abv => Roman Lygin
2016-01-25 10:25 abv Status assigned => feedback
2016-01-25 15:57 Roman Lygin Note Added: 0050102
2016-01-25 15:57 Roman Lygin Assigned To Roman Lygin => abv
2016-01-25 15:57 Roman Lygin Status feedback => assigned
2016-02-16 11:40 kgv Note Added: 0050799
2016-02-16 11:40 kgv Assigned To abv => bugmaster
2016-02-16 11:40 kgv Status assigned => resolved
2016-02-16 11:40 kgv Status resolved => reviewed
2016-02-16 11:41 kgv Relationship added related to 0027111
2016-02-19 10:34 bugmaster Status reviewed => tested
2016-02-19 10:38 bugmaster Note Added: 0050991
2016-02-21 08:52 abv Changeset attached => occt master a9dde4a3
2016-02-21 08:52 abv Assigned To bugmaster => abv
2016-02-21 08:52 abv Status tested => verified
2016-02-21 08:52 abv Resolution open => fixed
2016-04-17 13:18 git Note Added: 0052838
2016-04-20 15:43 aiv Fixed in Version => 7.0.0
2016-04-20 15:50 aiv Status verified => closed