Cppcoreguidelines: Flagging all uses of static downcasts seems overly draconian, and produces false positives.

[villevoutilainen]

In its enforcement,
C.146: Use dynamic_cast where class hierarchy navigation is unavoidable
states
"Enforcement: Flag all uses of static_cast for downcasts, including C-style casts that perform a static_cast."

What about static downcasts that are on the scale between almost-always-safe to guaranteed-to-be-safe?
I'm specifically talking about CRTP base templates. The enforcement of this rule is a false positive
there.

[hsutter]

Add an Exception to document where you might violate this rule, including CRTP and where the user has correctly calculated the correct type by a different means. Note that in both cases this is still a known bug source.

[AndrewPardoe]

Discussed 11/30/2015.

[hsutter]

Also add note that human attention is needed because the type system can't verify them.

[AndrewPardoe]

Discussed on 21 Dec.

[AndrewPardoe]

Note that an "Exception" exists in C.146. We can add some text about CRTP.

[villevoutilainen]

For what it's worth, here's an example of this guideline and its enforcement going down in flames. As a side anecdote, very few reviews in that project get multiple "absolutely not" responses. https://codereview.qt-project.org/#/c/254882/

[hsutter]

Thank you, this is very helpful feedback. Trying to capture a summary here in this thread:

The proposed 1-line change was to change this macro from static_cast to dynamic_cast:

#define qApp (static_cast<QApplication *>(QCoreApplication::instance()))

Note QCoreApplication::instance() returns a QCoreApplication*, which is a base class of QApplication.

The primary rationale for rejecting this was, from the comment thread:

Nope. dynamic_cast<> is nothing we use in Qt (it doesn't work when compiling without rtti). In addition, the cast is perfectly ok and safe, and dynamic_cast<> is a lot slower.

@villevoutilainen , I think these are three objections. Can you please confirm/clarify:

• "Compiling without RTTI": Is it correct that Qt is required to compile with with RTTI disabled (e.g., -fno-rtti-data, /GR-), and therefore cannot use dynamic_cast for that reason? For just this objection, would it be reasonable to #ifdef on whether RTTI is enabled and use dynamic_cast when it is enabled (if the other two objections were addressed)?

• "Perfectly ok and safe": Can you explain why the cast is both necessary and considered to be always safe? For example, if it's always safe, is there a reason why ::instance() doesn't return QApplication*?

• "A lot slower: Is the performance of dynamic_cast by itself a significant reason to reject changing this case to dynamic_cast (even if the other two objections were addressed)? If yes, is this primarily on one or two compilers/platforms, or pretty much a general concern on many compilers/platforms?

Editors, note that our guidance is the programmer should explicitly suppress a warning if they believe they know better, but this option was explicitly rejected... from the comment thread:

I don't think it's just a matter of nolint, it's also a matter of not having all these warnings in the first place. If I have a static_cast in my codebase that is 100% legitimate, last thing I want is to modify my code to make the IDE happy...

[hsutter]

Reopening...

[MikeGitb]

I find that statement pretty curious:

I don't think it's just a matter of nolint, it's also a matter of not having all these warnings in the first place. If I have a static_cast in my codebase that is 100% legitimate, last thing I want is to modify my code to make the IDE happy...

Isn't a situation where I, the developer, have knowledge that the analyzer can't have the exact use case for manual annotations/suppressions?

Of course this must not end in a situation where one has to provide 10 different annotations to satisfy every lint tool out there. Do we have a unified syntax to suppress core guideline issues by now?

The other question is of course, why this is a macro instead of an inline function (or - as herb suggested - part of ::instance() itself), but I guess changing that would be a breaking change.

For what it's worth: I prefer simple rules with low, but non-zero false positives rates that I can suppress in code (signaling my coworkers to apply special care when reviewing that part of the code) over complicated rules that are difficult to teach, implement and/or have high false negative rates.

Regarding the crtp usecase mentioned: Virtually al implementations of crtp I've seen had a as_derived member function anyway that encapsulated the cast. You can simply put a suppression in there and be done with it.

[villevoutilainen]

Pardon the delay, traveling, responding to the bullets:

@villevoutilainen , I think these are three objections. Can you please confirm/clarify:

* **"Compiling without RTTI":** Is it correct that Qt is required to compile with with RTTI disabled (e.g., `-fno-rtti-data`, `/GR-`), and therefore cannot use `dynamic_cast` for that reason? For just this objection, would it be reasonable to `#ifdef` on whether RTTI is enabled and use `dynamic_cast` when it is enabled (if the other two objections were addressed)?

Yes, it's correct that Qt is required to compile with RTTI disabled. As to whether using either
static_cast or dynamic_cast based on whether RTTI is enabled is reasonable.. well, purely
technically it might be remotely reasonable. There's arguably very VERY weak reasons
to bother with doing that conditionalizing, because the static_cast works fine for
all build configurations. It's a tall order to introduce an #ifdef like that mostly just
to make a relatively new linter-tool's rather controversial rule happy with code that
has no real problems in it. It would be adding
complexity to hide a false positive. I can't sell such a rationale for a patch.

* **"Perfectly ok and safe":** Can you explain why the cast is both necessary and considered to be always safe? For example, if it's always safe, is there a reason why `::instance()` doesn't return `QApplication*`?

Such a change would be an ABI break of potentially cosmic proportions. We cannot and will not do that
within a series of minor releases, and perhaps not even between major releases, because the
rationale for doing so is questionable. Furthermore, this particular ::instance does need to
return QCoreApplication* - it's just some users of it that know that it can be safely downcast, and those
users are the users who are expected to use the qApp macro. The "always safe" claim is not entirely
correct; it's correct by convention.

* **"A lot slower:** Is the performance of `dynamic_cast` by itself a significant reason to reject changing this case to `dynamic_cast` (even if the other two objections were addressed)? If yes, is this primarily on one or two compilers/platforms, or pretty much a general concern on many compilers/platforms?

Primarily on few compilers/platforms (if any), not many. Some of it may be unsubstantiated historical
fear.

[hsutter]

@villevoutilainen Thanks. Will digest.

Let me give some background...

Part of my personal concern here is that I've spent the last few years seeing a class of exploitable security vulnerabilities where the root cause has been exactly "they downcasted but didn't use dynamic_cast." The reasons they didn't use dynamic_cast have typically been that RTTI was not enabled (because it was too expensive, or by project convention, etc.) and/or that dynamic_cast was too slow (they felt they could write a better tagged down-cast by hand, or just blindly used static_cast because they knew it would be okay).

So these are people who used static_cast because they knew it would be correct by convention (as you say here), or had rolled their own type tagging system (because they knew it would be more efficient), and "knew" their code was correct... and were wrong, and hit failures and exploits (some they discovered themselves later, more often ones that had to be reported to them from production failures). In this class of failures, the code would have been correct had they used dynamic_cast, yet they are not using standard facilities. So I'm continuing to dig into root causes for why the standard facilities are not being used, and how to get people to use them (possibly by fixing the standard facilities).

This is the reason why this is in the Guidelines. You can blame me and Gaby more than the other editors for this being here, we continue to feel this pressure keenly from security folks, and the problem has not gone away. And I think it's really sad that people cannot use standard facilities, especially ones that would have prevented a class of bug.

So when I saw your note, I wanted to ask you more about it, because what I saw in that thread sounded like exactly the same pattern that I've been hearing for some time in these other type confusion bug cases. All of the people who write that code were sure their code is correct.

A suggestion to consider: Maybe you could do the experiment of changing this macro to dynamic_cast (if that's feasible in isolation) for only your internal test builds, just to see if you get unexpected type confusion bugs. (And, even better, if you ever compile your customers' code you can do the same and see if they accidentally inherit in an unexpected way that breaks the assumptions.) Just a suggestion though, you know your system and it may not be feasible to try out, or you may already have other safeguards that guarantee (not just by convention) that type confusion won't occur.

Thanks,

Herb

[villevoutilainen]

We can't realistically recompile our users' and customers' code, we simply don't have it available for such recompilation experiments. It's reasonable to expect that direct uses of QCoreApplication instead of QApplication are between relatively to extremely rare - that's the "by convention" part. Users who need QCoreApplication without the burden of QApplication are expected to know what they are doing, and are also expected to know not to use qApp. The rest is left to sanitizers rather than static checkers, in the imperfect world we live in.

[phillipjohnston]

In this class of failures, the code would have been correct had they used dynamic_cast, yet they are not using standard facilities. So I'm continuing to dig into root causes for why the standard facilities are not being used, and how to get people to use them (possibly by fixing the standard facilities).

I write embedded C++ code for relatively constrained systems. RTTI is almost always disabled. The nominal reason is to reduce binary size. Now, I realized as I was typing this that I've never actually quantified this oft-repeated advice to check its validity.

I ran a quick test for a firmware project I have, compiled with the latest gcc-arm-none-eabi with -O2 and typical linker optimization flags (function/data sections, strip unused sections). I am using libc++ with some minor modifications that I maintain for embedded purposes. My demo program uses polymorphism and static_cast<> in places where the guidelines recommend dynamic_cast<>.

There's about a 6% size difference for this application, with the only change being removal of -fno-rtti. I didn't update static_cast to dynamic_cast yet, so this is just an initial data point. Can't promise a timeline, but I'll work on a deeper investigation to quantify the impact of RTTI and see whether I can justify my continued advocation for disabling RTTI.

That being said, I've had to perform some real magic feats to fit new features in a fully-loaded system. 6% is a significant space savings.

---

No RTTI:

-rwxr-xr-x   1 pjohnston  staff  4786544 May  2 14:29 fwdemo_nrf52dk
-rwxr-xr-x   1 pjohnston  staff    62744 May  2 14:29 fwdemo_nrf52dk.bin

RTTI:

-rwxr-xr-x   1 pjohnston  staff  4841896 May  2 14:42 fwdemo_nrf52dk
-rwxr-xr-x   1 pjohnston  staff    66956 May  2 14:42 fwdemo_nrf52dk.bin