Fix uncaught exceptions by GilesBathgate · Pull Request #5233 · CGAL/cgal

GilesBathgate · 2020-11-30T21:16:34Z

Summary of Changes

In a previous issue ( #451 ) a CGAL_destructor_assertion macro was added to handle assertions made within destructors. When an exception is thrown from a destructor while the stack is being unwound, the existence of any uncaught exceptions cause the program to terminate immediately, without correct error handling. Sometimes it's safe to throw an exception even while std::uncaught_exception() == true. For example, if stack unwinding causes an object to be destructed, the destructor for that object could run code that throws an exception as long as the exception is caught by some catch block before escaping the destructor. That said coverity static analysis has found several places in the codebase where this is not the case, i.e the destructor calls some code which makes a CGAL_assertion() of some form or another, but doesn't catch the exception.

I propose a new macro:

#define CGAL_destructor_assertion_catch(CODE) try{ CODE } catch(...) { if(std::uncaught_exceptions() <= 0) throw; }

This preserves the assertion exception by rethrowing it if the stack is not being unwound and there are no uncaught exceptions. But in the case where we are recovering from an exception already, only that exception is preserved. A program which demonstrates this behaviour can be seen here: http://cpp.sh/6p7ka

This macro has then been applied to all places identified by static analysis as potentially throwing exceptions from CGAL_assertions. As before ( in #451 ) the CGAL_NOEXCEPT(CGAL_NO_ASSERTIONS_BOOL) macro has to be applied, where needed.

An additional Macro CGAL_destructor_warning was also added, this replicates the behaviour of CGAL_destructor_assertion, except emits a ::CGAL::warning_fail

Release Management

Affected package(s): several.
Feature/Small Feature (if any): bugfix
Link to compiled documentation (obligatory for small feature). I will add documentation if in principle the idea is acceptable.
License and copyright ownership: Returned to CGAL authors

lrineau · 2020-12-14T08:28:28Z

HalfedgeDS/include/CGAL/HalfedgeDS_list.h

+    ~HalfedgeDS_list() noexcept {
+      try {
+        clear();
+      } catch (...) {}
+    }


How is it possible that a clear() function can trigger? What does it do?!

... Oh... is it about assertions in the clear() functions, that can trigger?

precisely, they are all from CGAL_assertion. I didn't think the overhead of this code in release builds was significant, to warrant using CGAL_assertion_code

maxGimeno · 2021-03-03T10:23:21Z

There are conflicts with master

lrineau · 2021-03-25T09:08:38Z

This PR, and particularly 9f9e1e2 is the one responsible for all the compilation errors about boost::any destructor, in the last test results:

/usr/include/boost/any.hpp: In instantiation of 'class boost::any::holder<CGAL::Segment_2<CGAL::Cartesian<double> > >':
/usr/include/boost/any.hpp:253:17:   required from 'ValueType* boost::any_cast(boost::any*) [with ValueType = CGAL::Segment_2<CGAL::Cartesian<double> >]'
/mnt/testsuite/include/CGAL/Object.h:87:43:   required from 'bool CGAL::Object::assign(T&) const [with T = CGAL::Segment_2<CGAL::Cartesian<double> >]'
/mnt/testsuite/include/CGAL/Object.h:157:20:   required from 'bool CGAL::assign(T&, const CGAL::Object&) [with T = CGAL::Segment_2<CGAL::Cartesian<double> >]'
.../test/Triangulation_2/include/CGAL/_test_cls_delaunay_triangulation_2.h:379:24:   required from 'void _test_delaunay_duality(const Del&) [with Del = CGAL::Delaunay_triangulation_2<CGAL::Cartesian<double> >]'
.../test/Triangulation_2/include/CGAL/_test_cls_delaunay_triangulation_2.h:201:26:   required from 'void _test_cls_delaunay_triangulation_2(const Del&) [with Del = CGAL::Delaunay_triangulation_2<CGAL::Cartesian<double> >]'
.../test/Triangulation_2/test_delaunay_triangulation_2.cpp:70:46:   required from here
/usr/include/boost/any.hpp:169:15: error: looser exception specification on overriding virtual function 'virtual boost::any::holder<CGAL::Segment_2<CGAL::Cartesian<double> > >::~holder() noexcept (false)'
  169 |         class holder
      |               ^~~~~~
/usr/include/boost/any.hpp:156:21: note: overridden function is 'virtual boost::any::placeholder::~placeholder() noexcept'
  156 |             virtual ~placeholder()
      |                     ^

GilesBathgate · 2021-03-25T09:22:08Z

This PR, and particularly 9f9e1e2 is the one responsible for all the compilation errors about boost::any destructor

Ah, yes. When the class inherits from another that has a noexcept destructor, it cannot use CGAL_NOEXCEPT(CGAL_NO_ASSERTIONS_BOOL) in fact, the destructor must be noexcept too, which means consequently that the destructor can never throw. Therefore the only sensible pattern that applies is:

~Handle_for() noexcept
    {
      try {
        if (--(ptr_->count) == 0) {
          Allocator_traits::destroy(allocator, ptr_);
          allocator.deallocate( ptr_, 1);
        }
      } catch(...) {}
    }

This means you lose the exception in the destructor entirely, but its better than program termination which is what happens when you throw from a noexcept method.

GilesBathgate · 2021-03-25T13:45:38Z

@lrineau Fixed as I proposed above.

maxGimeno · 2021-03-25T14:20:32Z

@GilesBathgate Where can we see your fix ? The force push kind of hides the modifications
EDIT: Nevermind, I found it.

GilesBathgate · 2021-03-25T14:43:15Z

@GilesBathgate Where can we see your fix ? The force push kind of hides the modifications
EDIT: Nevermind, I found it.

48831ab

maxGimeno · 2021-03-25T14:47:09Z

Thanks !

maxGimeno · 2021-04-06T12:43:27Z

https://cgal.geometryfactory.com/CGAL/testsuite/results-5.3-Ic-98.shtml

mglisse · 2021-04-19T11:50:48Z

STL_Extension/include/CGAL/Handle_for.h

    }

-    ~Handle_for()
+    ~Handle_for() noexcept


That's useless, it is noexcept by default.

It's probably a hang-over from when instead I'd had CGAL_NOEXCEPT(CGAL_NO_ASSERTIONS_BOOL)
I've since found some more uncaught exception issues with the static analysis on the latest runs (where I upgraded from 5.0.3 to master) I will clean this up along with the others and put them in a new PR.

mglisse · 2021-04-19T14:59:23Z

STL_Extension/include/CGAL/Handle_for.h

+      try{
+        if (--(ptr_->count) == 0) {
+          Allocator_traits::destroy(allocator, ptr_);
+          allocator.deallocate( ptr_, 1);
+        }
+      } catch(...) {}


I am not going to fight it, but I want to mention that I am not fond of this new code

it is ugly

it goes against the default specified by the language

Many of the destructors in the CGAL codebase can throw because of a CGAL_assertion, so when it calls destroy/deallocate here the program will terminate. (presumably, that's not desirable, that's what the change intends to prevent)

In most places, this is handled by a CGAL_destructor_assertion_catch which preserves the exception when possible. But here because the noexcept is enforced by a base class we can only swallow the exception.

However you might agree that the correct way to solve all these problems, is never to throw from a destructor in the first place. Then there wouldn't have to be workarounds for the default behaviour specified by the language.

Many of the destructors in the CGAL codebase can throw because of a CGAL_assertion, so when it calls destroy/deallocate here the program will terminate. (presumably, that's not desirable, that's what the change intends to prevent)

It isn't obvious that it isn't desirable. The standard didn't gratuitously introduce std::terminate, it was better in those circumstances than data corruption or security holes.

In most places, this is handled by a CGAL_destructor_assertion_catch which preserves the exception when possible. But here because the noexcept is enforced by a base class we can only swallow the exception.

However you might agree that the correct way to solve all these problems, is never to throw from a destructor in the first place. Then there wouldn't have to be workarounds for the default behaviour specified by the language.

And assertions are not supposed to trigger, so that's good, no need to introduce workarounds for the case where they trigger 😉
Presumably, we aren't checking preconditions on user input in those destructors. If we assert, it probably means a bug in CGAL, or a side effect of some memory corruption, anyway something bad. We can disable those checks, as your patch does, that makes your checker happy and possibly fails to detect an issue. Or we can have checks that abort if they cannot throw exceptions, which is what the language specifies.

I wonder if separating more clearly debug assertions from checks that can make sense for users would reduce the number of destructors affected.

Some assertions are meant to protect from users who don't know what they are doing, say a user constructing Interval_nt(3,2). CGAL never constructs an interval with a>b, but since CGAL and the user's code call the same constructor, they go through the same assertion, and it looks like the CGAL could throw although it won't. If this happens in a noexcept function (like a destructor), the compiler has to generate instructions on what to do with that non-existing exception, for instance ignore it or abort. The amount of code is essentially the same, there shouldn't be much impact on performance, and since the exception never happens, the behavior is also the same. One code is simpler (no need for try-catch), and on the off chance some memory corruption causes the assertion to trigger, it will safely abort instead of executing random instructions.

Anyway, I said I won't fight...

I appreciate your input @mglisse . It's always worthwhile having a discussion even if there are differences in opinion.

Presumably, we aren't checking preconditions on user input in those destructors.

Not a destructor, but here is a noexcept method with a CGAL_precondition

cgal/STL_Extension/include/CGAL/Handle.h

Lines 70 to 79 in 77e5d1d

Handle&

operator=(const Handle& x) noexcept

{

CGAL_precondition( x.PTR != static_cast<Rep*>(0) );

x.PTR->count++;

if ( PTR && (--PTR->count == 0))

delete PTR;

PTR = x.PTR;

return *this;

}

Not a destructor, but here is a noexcept method with a CGAL_precondition

Yes, I think I remember adding this noexcept and wondering about the precondition. In the end, I was ok with having the program terminate if a null pointer was found in debug mode. The precondition isn't there in release mode. Not testing in debug mode would be a regression. Making the function conditionally noexcept would have been an option, but it can change the path taken by the code, and it felt more useful to lie a bit and have debug mode test something closer to release mode, than to insist on propagating the error as an exception. Of course different opinions are always possible.

Making the function conditionally noexcept

In our usage of the CGAL library (RapCAD and OpenSCAD) we enable CGAL_precondition in release mode, so this would be more suitable for us I think.

Can't you check the prerequisites in inputs before calling the functions? Or is it too expensive compared to do a try-catch?

Can't you check the prerequisites in inputs before calling the functions?

Not sure I follow what you mean @sloriot

GilesBathgate added 10 commits November 28, 2020 14:17

Add a destructor assertion catch macro

78bbe15

Fix uncaught exception in K3_tree.h

96024d1

Fix uncaught exception in VRML_2_ostream.h

9d442f2

Fix uncaught exception in Sphere_map.h

35c1480

Fix uncaught exception in Nef_polyhedron_S2.h

e222a42

Fix uncaught exception in SNC_point_locator.h

ce26378

Fix uncaught exception in Handle_for.h

48831ab

Fix uncaught exception in HalfedgeDS_list.h

1373150

Add noexcept to Ref_counted_base in Straight_skeleton_aux.h

a7abec0

Fix uncaught exception in In_place_list.h

e7aebc4

maxGimeno added this to the 5.3-beta milestone Dec 2, 2020

lrineau reviewed Dec 14, 2020

View reviewed changes

lrineau self-assigned this Dec 14, 2020

maxGimeno added the Conflicts label Mar 3, 2021

GilesBathgate force-pushed the fix-uncaught-exceptions branch from 0f03798 to cdd577d Compare March 3, 2021 11:24

lrineau removed the Conflicts label Mar 3, 2021

maxGimeno added Ready to be tested Under Testing labels Mar 24, 2021

lrineau mentioned this pull request Mar 25, 2021

Arrangement: Use Compact Container for DCEL #5408

Merged

lrineau added Tests failing and removed Under Testing labels Mar 25, 2021

GilesBathgate force-pushed the fix-uncaught-exceptions branch from cdd577d to 1373150 Compare March 25, 2021 09:48

maxGimeno added Under Testing and removed Tests failing labels Mar 25, 2021

maxGimeno added the Tested label Apr 6, 2021

lrineau added rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' and removed Ready to be tested Under Testing labels Apr 6, 2021

lrineau merged commit 31b817c into CGAL:master Apr 6, 2021

lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Apr 7, 2021

mglisse reviewed Apr 19, 2021

View reviewed changes

GilesBathgate deleted the fix-uncaught-exceptions branch April 19, 2021 14:50

mglisse reviewed Apr 19, 2021

View reviewed changes

GilesBathgate mentioned this pull request Apr 21, 2021

Fix uncaught exceptions cleanup #5631

Merged

lrineau added Merged_in_5.3-beta1 Merged_in_5.3 labels Jul 2, 2021

	Handle&
	operator=(const Handle& x) noexcept
	{
	CGAL_precondition( x.PTR != static_cast<Rep*>(0) );
	x.PTR->count++;
	if ( PTR && (--PTR->count == 0))
	delete PTR;
	PTR = x.PTR;
	return *this;
	}

Conversation

GilesBathgate commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Release Management

Uh oh!

lrineau Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GilesBathgate Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxGimeno commented Mar 3, 2021

Uh oh!

lrineau commented Mar 25, 2021

Uh oh!

GilesBathgate commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GilesBathgate commented Mar 25, 2021

Uh oh!

maxGimeno commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GilesBathgate commented Mar 25, 2021

Uh oh!

maxGimeno commented Mar 25, 2021

Uh oh!

maxGimeno commented Apr 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GilesBathgate Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GilesBathgate Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GilesBathgate Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GilesBathgate commented Nov 30, 2020 •

edited

Loading

lrineau Dec 14, 2020 •

edited

Loading

GilesBathgate Dec 14, 2020 •

edited

Loading

GilesBathgate commented Mar 25, 2021 •

edited

Loading

maxGimeno commented Mar 25, 2021 •

edited

Loading

GilesBathgate Apr 19, 2021 •

edited

Loading

GilesBathgate Apr 20, 2021 •

edited

Loading

GilesBathgate Apr 21, 2021 •

edited

Loading