MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0032181Community[OCCT] OCCT:Modeling Algorithmspublic2021-02-27 20:472021-03-06 13:25
Reporterchennes 
Assigned Tobugmaster 
PrioritynormalSeverityminor 
StatusverifiedResolutionfixed 
PlatformLinuxOSDebian 6.0OS Version64 bit
Product Version[OCCT] 7.5.0 
Target Version[OCCT] 7.6.0*Fixed in Version 
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? file icon Crash-MA-V1.brep (264,301 bytes) 2021-03-02 19:42

- Relationships

-  Notes
(0099226)
git (administrator)
2021-02-28 01:40

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.
(0099228)
git (administrator)
2021-02-28 21:26

Branch CR0032181 has been updated forcibly by chennes.

SHA-1: 60f6e4f74d54af84781d73f2a97b0c9a11a4831e
(0099253)
msv (developer)
2021-03-01 21:16

I have run the tests. http://jenkins-test-12.nnov.opencascade.com/view/CR0032181-master-msv/view/COMPARE/ [^]
(0099254)
msv (developer)
2021-03-01 21:41
edited on: 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".

(0099255)
msv (developer)
2021-03-01 21:56
edited on: 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

(0099256)
msv (developer)
2021-03-01 22:26

40 test cases are failed with this patch. It is needed to analyze the causes and fix.
(0099259)
git (administrator)
2021-03-02 06:07

Branch CR0032181 has been updated forcibly by chennes.

SHA-1: 4f4a2a86a622c9db82e6444d673950d2987e366d
(0099260)
chennes (developer)
2021-03-02 06:47

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.
(0099272)
msv (developer)
2021-03-02 10:56

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.
(0099314)
git (administrator)
2021-03-02 18:23

Branch CR0032181 has been updated forcibly by msv.

SHA-1: 40c6559891eab30461680e2a5b7ec043fbc9a709
(0099315)
msv (developer)
2021-03-02 18:25

I have changed the patch making it safer. The tests are restarted.
(0099316)
msv (developer)
2021-03-02 19:52

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.
(0099317)
msv (developer)
2021-03-02 19:56

The updated patch doesn't produce regressions.
(0099318)
msv (developer)
2021-03-02 19:57

For integration:
occt - CR0032181
products - none
(0099320)
msv (developer)
2021-03-02 20:00

Review, test, debug.
(0099471)
bugmaster (administrator)
2021-03-06 12:15

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
(0099495)
git (administrator)
2021-03-06 13:25

Branch CR0032181 has been deleted by inv.

SHA-1: 40c6559891eab30461680e2a5b7ec043fbc9a709

- Related Changesets
occt: master 329e5df9
Timestamp: 2021-02-27 17:58:50
Author: 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.
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 View Revisions
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 View Revisions
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 View Revisions
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


Copyright © 2000 - 2021 MantisBT Team
Powered by Mantis Bugtracker