MantisBT - Community
View Issue Details
0027104Community[OCCT] OCCT:Foundation Classespublic2016-01-21 16:282016-04-20 15:50
Roman Lygin 
abv 
normalmajor 
closedfixed 
[OCCT] 7.0.0 
[OCCT] 7.0.0[OCCT] 7.0.0 
0027104: DownCast() cannot return null for mismatched handle
Downcast() 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.
See the attached C++ reproducer which is a synthetic reproducer of a case coming from a real-world IGES file.
No tags attached.
related to 0024023closed abv Open CASCADE Revamp the OCCT Handle 
related to 0026377closed abv Open CASCADE Passing Handle objects as arguments to functions as non-const reference to base type is dangerous 
related to 0027111closed abv Community Add generalized copy constructor in handle class for old compilers 
cxx main.cxx (2,358) 2016-01-21 16:28
https://tracker.dev.opencascade.org/
Issue History
2016-01-21 16:28Roman LyginNew Issue
2016-01-21 16:28Roman LyginAssigned To => abv
2016-01-21 16:28Roman LyginFile Added: main.cxx
2016-01-21 16:30Roman LyginRelationship addedrelated to 0024023
2016-01-21 16:31Roman LyginRelationship addedrelated to 0026377
2016-01-21 17:51abvTarget Version7.1.0 => 7.0.0
2016-01-22 15:52abvNote Added: 0050064
2016-01-22 15:52abvAssigned Toabv => Roman Lygin
2016-01-22 15:52abvStatusnew => feedback
2016-01-22 16:58Roman LyginNote Added: 0050071
2016-01-22 16:58Roman LyginAssigned ToRoman Lygin => abv
2016-01-22 16:58Roman LyginStatusfeedback => assigned
2016-01-22 19:25abvNote Added: 0050079
2016-01-25 10:09abvSummary[Regression in 7.0 beta] DownCast() cannot return null for mismatched handle => DownCast() cannot return null for mismatched handle
2016-01-25 10:18gitNote Added: 0050097
2016-01-25 10:25abvNote Added: 0050098
2016-01-25 10:25abvAssigned Toabv => Roman Lygin
2016-01-25 10:25abvStatusassigned => feedback
2016-01-25 15:57Roman LyginNote Added: 0050102
2016-01-25 15:57Roman LyginAssigned ToRoman Lygin => abv
2016-01-25 15:57Roman LyginStatusfeedback => assigned
2016-02-16 11:40kgvNote Added: 0050799
2016-02-16 11:40kgvAssigned Toabv => bugmaster
2016-02-16 11:40kgvStatusassigned => resolved
2016-02-16 11:40kgvStatusresolved => reviewed
2016-02-16 11:41kgvRelationship addedrelated to 0027111
2016-02-19 10:34bugmasterStatusreviewed => tested
2016-02-19 10:38bugmasterNote Added: 0050991
2016-02-21 08:52abvChangeset attached => occt master a9dde4a3
2016-02-21 08:52abvAssigned Tobugmaster => abv
2016-02-21 08:52abvStatustested => verified
2016-02-21 08:52abvResolutionopen => fixed
2016-04-17 13:18gitNote Added: 0052838
2016-04-20 15:43aivFixed in Version => 7.0.0
2016-04-20 15:50aivStatusverified => closed

Notes
(0050064)
abv   
2016-01-22 15:52   
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.
(0050071)
Roman Lygin   
2016-01-22 16:58   
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.
(0050079)
abv   
2016-01-22 19:25   
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()).
(0050097)
git   
2016-01-25 10:18   
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).
(0050098)
abv   
2016-01-25 10:25   
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).
(0050102)
Roman Lygin   
2016-01-25 15:57   
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.

(0050799)
kgv   
2016-02-16 11:40   
Related modification within CR27111_6 has been reviewed without remarks.
Should be tested in scope of 0027111.
(0050991)
bugmaster   
2016-02-19 10:38   
Tested in framework of branch CR27111_6
(0052838)
git   
2016-04-17 13:18   
Branch CR27104 has been deleted by kgv.

SHA-1: 441358d586e0e5abf1e5718c879735d821c0ebf8