View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0032181CommunityOCCT:Modeling Algorithmspublic2021-02-27 20:472021-03-06 13:25
Reporterchennes Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
PlatformLinuxOSDebian 6.0 
Product Version7.5.0 
Target Version7.6.0Fixed in Version7.6.0 
Summary0032181: Modeling Algorithms - ChFi3d missing error checking
DescriptionThroughout the ChFi3d fillet creation functions there are two sets of unchecked errors that can result in segmentation faults when geometry-creation problems occur.

The first is that the functions ChFi3d_cherche_element and ChFi3d_cherche_face1 may fail to find the requested item: they silently return without setting the required reference, which is then accessed by the algorithms. This can be caught with a conditional at the end of the function that throws a Standard_Failure if the required item was not found.

The second class of unchecked error is the use of BRep_Tool::CurveOnSurface, which may fail to create the required curve, returning a null handle. In no cases in the existing fillet code is this checked. This can be dealt with by checking the returned handle using IsNull() and throwing a Standard_Failure if the call did not result in valid geometry.

These issues were identified by various users of FreeCAD. For the types of geometry that result in triggering various segmentation faults see--
https://tracker.freecadweb.org/view.php?id=3142
https://tracker.freecadweb.org/view.php?id=3310
https://tracker.freecadweb.org/view.php?id=3970
https://tracker.freecadweb.org/view.php?id=4076
https://tracker.freecadweb.org/view.php?id=4078
https://tracker.freecadweb.org/view.php?id=4291
https://tracker.freecadweb.org/view.php?id=4319
https://tracker.freecadweb.org/view.php?id=4321
https://tracker.freecadweb.org/view.php?id=4487
https://tracker.freecadweb.org/view.php?id=4518
https://tracker.freecadweb.org/view.php?id=4543
Steps To ReproduceThis patch resolves the following FreeCAD issues:
https://tracker.freecadweb.org/view.php?id=3142 [^]
https://tracker.freecadweb.org/view.php?id=3310 [^]
https://tracker.freecadweb.org/view.php?id=3970 [^]
https://tracker.freecadweb.org/view.php?id=4076 [^]
https://tracker.freecadweb.org/view.php?id=4078 [^]
https://tracker.freecadweb.org/view.php?id=4291 [^]
https://tracker.freecadweb.org/view.php?id=4319 [^]
https://tracker.freecadweb.org/view.php?id=4321 [^]
https://tracker.freecadweb.org/view.php?id=4487 [^]
https://tracker.freecadweb.org/view.php?id=4518 [^]
https://tracker.freecadweb.org/view.php?id=4543 [^]
TagsNo tags attached.
Test case numberNot required

Attached Files

  • Crash-MA-V1.brep (264,301 bytes)

Activities

git

2021-02-28 01:40

administrator   ~0099226

Branch CR0032181 has been created by chennes.

SHA-1: 81385a0604caaa075a4e2b19b0ff1c04c30492c5


Detailed log of new commits:

Author: Chris Hennes (chennes)
Date: Sat Feb 27 11:58:50 2021 -0600

    0032181: Modeling Algorithms - ChFi3d missing error checking
    
    Throughout the ChFi3d fillet creation functions there are two sets of unchecked
    errors that can result in segmentation faults when geometry-creation problems
    occur.
    
    The first is that the functions ChFi3d_cherche_* may fail to find the requested
    item: they silently return without setting the required reference, which is
    then accessed by the algorithms. This can be caught with a conditional at the
    end of the function that throws a Standard_Failure if the required item was not
    found.
    
    The second class of unchecked error is the use of BRep_Tool::CurveOnSurface,
    which may fail to create the required curve, returning a null handle. In many
    cases in the existing fillet code this is not checked. This can be dealt with
    by checking the returned handle using IsNull() and throwing a Standard_Failure
    if the call did not result in valid geometry.

git

2021-02-28 21:26

administrator   ~0099228

Branch CR0032181 has been updated forcibly by chennes.

SHA-1: 60f6e4f74d54af84781d73f2a97b0c9a11a4831e

msv

2021-03-01 21:16

developer   ~0099253

I have run the tests. http://jenkins-test-12.nnov.opencascade.com/view/CR0032181-master-msv/view/COMPARE/

msv

2021-03-01 21:41

developer   ~0099254

Last edited: 2021-03-01 21:42

 
+  if (gpcprol.IsNull()) throw Standard_ConstructionError ("Failed to create curve.");

Please use the OCCT coding rules (https://dev.opencascade.org/doc/overview/html/occt_contribution__coding_rules.html#occt_coding_rules_3). In particular, here it should be:

+  if (gpcprol.IsNull())
+    throw Standard_ConstructionError ("Failed to create curve.");

Taking into account that BRep_Tool::CurveOnSurface does not create a curve, but rather retrieves the available one from the edge, the message in the exception is better to write as "Failed to get p-curve of edge".
In the case of BRep_Tool::Curve, the message is to be "Failed to get 3D curve of edge".
In the case of Calcul_C2dOnFace, the message is to be "Failed to calculate curve on surface".

msv

2021-03-01 21:56

developer   ~0099255

Last edited: 2021-03-01 21:57

 Dear Chris,
Could you also attach here some test cases to reproduce the bug? It would be very good to add non-regression tests.
The better is to have a ready to run Draw script. If you cannot provide a script you can attach a shape and provide a piece of code to reproduce.
Thanks in advance.
Mikhail

msv

2021-03-01 22:26

developer   ~0099256

40 test cases are failed with this patch. It is needed to analyze the causes and fix.

git

2021-03-02 06:07

administrator   ~0099259

Branch CR0032181 has been updated forcibly by chennes.

SHA-1: 4f4a2a86a622c9db82e6444d673950d2987e366d

chennes

2021-03-02 06:47

developer   ~0099260

I have pushed an update making the requested formatting and language changes. When I run the blend test suite, I only get a single failure (blend simple V1) -- but I cannot run that test at all without this patch, it crashes draw on my system. I am unsure how to proceed from here.

msv

2021-03-02 10:56

developer   ~0099272

The test blend simple V1 run OK without this patch. And now it puts in output after tolblend command:
Tcl Exception: tolerance ang : 0.01
After that the test crashes.

I have looked at it with debugger and found out that the method ChFi3d_cherche_edge throws exception. Without the patch, exception here is not thrown, but it is not the problem for this case, as the edge E is not null on input.
I have added the condition E.IsNull() in the 'if' statement, and the test becomes OK.

git

2021-03-02 18:23

administrator   ~0099314

Branch CR0032181 has been updated forcibly by msv.

SHA-1: 40c6559891eab30461680e2a5b7ec043fbc9a709

msv

2021-03-02 18:25

developer   ~0099315

I have changed the patch making it safer. The tests are restarted.

msv

2021-03-02 19:42

developer  

Crash-MA-V1.brep (264,301 bytes)

msv

2021-03-02 19:52

developer   ~0099316

I have attached here the shape from the FreeCad bug 4543. I have tried to reproduce the bug making fillet on the edge 25:

restore Crash-MA-V1.brep a
explode a e
fillet r a 0.1 a_25


Without the patch, the signal AccessViolation is raised, and it is caught in the method ChFi3d_Builder::Compute(), due to handling signals as exceptions that is made in Draw by default by calling the method OSD_SetSignal(). In the result the operation is failed, but no crash in Draw.

With the patch, exception ConstructionError is thrown instead of the signal. It is good, as it seems, for FreeCad. For Draw the behavior is the same. So, it is impossible to create a non-regression test.

msv

2021-03-02 19:56

developer   ~0099317

The updated patch doesn't produce regressions.

msv

2021-03-02 19:57

developer   ~0099318

For integration:
occt - CR0032181
products - none

msv

2021-03-02 20:00

developer   ~0099320

Review, test, debug.

bugmaster

2021-03-06 12:15

administrator   ~0099471

Combination -
OCCT branch : IR-2021-03-05
master SHA - 58210e5983a7986bc4cd1bec9c0b5cb29828fda4
a87b7ddc8cb44606b91e3f37113847c3f5f50fdc
Products branch : IR-2021-03-05 SHA - b3d022cfefe721ee2ad6db68ca360345d656610b
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 17744.80000000016 / 17811.87000000015 [-0.38%]
Products
Total CPU difference: 11539.9800000001 / 11543.6500000001 [-0.03%]
Windows-64-VC14:
OCCT
Total CPU difference: 19321.640625 / 19344.390625 [-0.12%]
Products
Total CPU difference: 12868.796875 / 12900.578125 [-0.25%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

git

2021-03-06 13:25

administrator   ~0099495

Branch CR0032181 has been deleted by inv.

SHA-1: 40c6559891eab30461680e2a5b7ec043fbc9a709

Related Changesets

occt: master 329e5df9

2021-02-27 17:58:50

Chris Hennes (chennes)


Committer: bugmaster Details Diff		 0032181: Modeling Algorithms - ChFi3d missing error checking

Throughout the ChFi3d fillet creation functions there are two sets of unchecked
errors that can result in segmentation faults when geometry-creation problems
occur.

The first is that the functions ChFi3d_cherche_* may fail to find the requested
item: they silently return without setting the required reference, which is
then accessed by the algorithms. This can be caught with a conditional at the
end of the function that throws an exception if the required item was not
found.

The second class of unchecked error is the use of BRep_Tool::CurveOnSurface,
which may fail to create the required curve, returning a null handle. In many
cases in the existing fillet code this is not checked. This can be dealt with
by checking the returned handle using IsNull() and throwing an exception
if the call did not result in valid geometry.		 Affected Issues
0032181
mod - src/ChFi3d/ChFi3d_Builder_0.cxx Diff File
mod - src/ChFi3d/ChFi3d_Builder_C1.cxx Diff File
mod - src/ChFi3d/ChFi3d_Builder_CnCrn.cxx Diff File

Issue History

Date Modified Username Field Change
2021-02-27 20:47 chennes New Issue
2021-02-27 20:47 chennes Assigned To => chennes
2021-02-28 01:22 chennes Assigned To chennes =>
2021-02-28 01:22 chennes Assigned To => chennes
2021-02-28 01:40 git Note Added: 0099226
2021-02-28 10:03 kgv Target Version => 7.6.0
2021-02-28 10:03 kgv Summary ChFi3d missing error checking => Modeling Algorithms - ChFi3d missing error checking
2021-02-28 21:26 git Note Added: 0099228
2021-03-01 18:09 chennes Assigned To chennes => msv
2021-03-01 18:09 chennes Status new => resolved
2021-03-01 18:09 chennes Steps to Reproduce Updated
2021-03-01 21:16 msv Note Added: 0099253
2021-03-01 21:41 msv Note Added: 0099254
2021-03-01 21:42 msv Note Edited: 0099254
2021-03-01 21:44 msv Assigned To msv => chennes
2021-03-01 21:44 msv Status resolved => assigned
2021-03-01 21:56 msv Note Added: 0099255
2021-03-01 21:57 msv Note Edited: 0099255
2021-03-01 22:26 msv Note Added: 0099256
2021-03-02 06:07 git Note Added: 0099259
2021-03-02 06:47 chennes Note Added: 0099260
2021-03-02 10:56 msv Note Added: 0099272
2021-03-02 18:23 git Note Added: 0099314
2021-03-02 18:25 msv Note Added: 0099315
2021-03-02 18:30 msv Assigned To chennes => msv
2021-03-02 18:30 msv Status assigned => resolved
2021-03-02 19:42 msv File Added: Crash-MA-V1.brep
2021-03-02 19:52 msv Note Added: 0099316
2021-03-02 19:56 msv Note Added: 0099317
2021-03-02 19:57 msv Note Added: 0099318
2021-03-02 19:57 msv Assigned To msv => bugmaster
2021-03-02 19:57 msv Status resolved => reviewed
2021-03-02 20:00 msv Note Added: 0099320
2021-03-06 12:15 bugmaster Note Added: 0099471
2021-03-06 12:15 bugmaster Status reviewed => tested
2021-03-06 12:21 bugmaster Test case number => Not required
2021-03-06 12:34 bugmaster Changeset attached => occt master 329e5df9
2021-03-06 12:34 bugmaster Status tested => verified
2021-03-06 12:34 bugmaster Resolution open => fixed
2021-03-06 13:25 git Note Added: 0099495