New Defects reported by Coverity Scan for LibreOffice
Stephan Bergmann
sbergman at redhat.com
Tue Jan 24 08:44:26 UTC 2017
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)").
More information about the LibreOffice
mailing list