Re: New Defects reported by Coverity Scan for LibreOffice

On 01/23/2017 10:37 PM, Caolán McNamara wrote:
> On Mon, 2017-01-23 at 11:40 +0100, Stephan Bergmann wrote:
> > I don't think adding noexcept(false) to such dtors would be
> > good. They are surely not meant to throw.
>
> Well, take 1399220 as a common enough pattern, an OUString +=
> "something" and OUString::operator+= calls rtl_uString_newConcatAsciiL
> and that has a throw std::length_error. If it makes sense to claim this
> dtor won't throw cause that's just not going to happen in the real
> world, then it might make as much sense to remove the throw from +=
> instead.

I think the

>     if (left->length > std::numeric_limits<sal_Int32>::max() - rightLength) {
>         throw std::length_error("rtl_uString_newConcatAsciiL");
>     }

in rtl_uString_newConcatAsciiL (sal/rtl/ustring.cxx), can, for most 
practical purposes, be considered about equivalent to

>     if (left->length > std::numeric_limits<sal_Int32>::max() - rightLength) {
>         std::abort();
>     }

---the program will terminate abruptly when that condition is met, as 
there is usually no code written to handle that unanticipated condition.

However, this is obviously better than silently doing nothing or 
sticking a (debug-builds--only) assert there (not to mention that an 
assert would feel logically wrong, given rtl_uString_newConcatAsciiL's 
preconditions).  For a poorly written import filter, e.g., it may make 
much of a security difference (modulo poorly handled stack-unwinding 
effects, in the case of throw).

(And it probably wouldn't really help the Coverity case to replace such 
throw expressions with calls to std::abort or assert, anyway.  There's 
also calls to std library functions that may throw, and even if Coverity 
might not today use such to construct its warnings from, who knows it 
won't after the next update.)

So, what does that mean for dtors?  I think the most obvious take-away 
(and the best place where to spend energy) is to do as little as 
possible in dtors.  There's still some violations of that across the 
code base, and they keep biting us every now and then (e.g., when the 
dtor of a UNO object is only triggered by JVM GC, at unpredictable times).

Apart from that, if a dtor calls code that can throw, but the dtor's 
author doesn't reason whether or not such an exceptional condition can 
actually happen (which probably covers the vast majority of dtors ever 
written), I think the throw-is-moral-equivalent-of-std::abort failure 
handling covers this case best.  (Which implies that the dtor should not 
be marked noexcept(false), in the spirit of catching errors early.)

If, on the other hand, a dtor is carefully proven to never cause an 
exception to be thrown (from within a call it makes to a function that 
can potentially throw), the "Java-style" solution could be to rewrite 
that call as

  try {
    call();
  } catch (whatever::ex &) {
    assert(false); // cannot happen
  }

But do we want to pepper our code base with such clutter?

> Anyhow, if any member of a class or base-class has a dtor thats
> nothrow(false) that'll poison the whole hierarchy so they are
> implicitly nothrow(false) too, right ? So, in your dynamic exception
> specification changes mail you mentioned "For dtors, the dynamic
> exception specification would be replaced with an explicit
> nothrow(false)." so it might end up that lots of these warning melt
> away again if that affects something sufficiently lowlevel ? Maybe
> worry about these again after those changes land.

Dtors with a dynamic exception specification ("throw (foo)", /not/ 
merely a throw-nothing "throw ()") are exceedingly rare.  I haven't come 
across one in our code base yet, and the only existing noexcept(false) 
dtor is ~GErrorWrapper in 
shell/source/sessioninstall/SyncDbusSessionHelper.cxx (from 
<https://cgit.freedesktop.org/libreoffice/core/commit/?id=21f98618d8cd7562d423e94db988126d662165c9> 
"-Werror=terminate (GCC 6)").