Remove dynamic_cf_cast<>() and replace it with checked_cf_cast<>()

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[David Kilzer (:ddkilzer)]

Remove dynamic_cf_cast<>() and replace it with checked_cF_cast<>().

See discussion in Bug 210575, Comment #16: <https://bugs.webkit.org/show_bug.cgi?id=210575#c16>

Always checking for the correct type is the safest option, and it actually wasn't used in that many places anyway:

Source/WebCore/platform/cf/KeyedDecoderCF.cpp
Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp
Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm
Source/WebKit/UIProcess/mac/LegacySessionStateCoding.cpp

[David Kilzer (:ddkilzer)]

Created attachment 396936 [details]
Patch v1

[Darin Adler]

Comment on attachment 396936 [details]
Patch v1

I’m OK with this.

Are there any of these cases where it would be better if the wrong type was silently ignored instead of crashing?

Separately, I’m not sure we should remove dynamic_cf_cast. Some clients might have a reason to use it, just not in these decode functions. It’s the same concept as dynamic_objc_cast and dynamic_cast. I think instead we should consider removing the assertion from it instead of removing it entirely.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #2)
> Comment on attachment 396936 [details]
> Patch v1
> 
> I’m OK with this.
> 
> Are there any of these cases where it would be better if the wrong type was
> silently ignored instead of crashing?

I'm not sure I can answer that.

The only reason I could see for not crashing due to an incorrect (and unexpected) type is if some other framework consistently returns the wrong type of object and it has to be worked around so the crash doesn't occur every time.

Otherwise, getting an unexpected type seems crash-worthy from a security/defensive posture.  It should never happen, so it's okay to crash when it does because it means something went horribly wrong.

> Separately, I’m not sure we should remove dynamic_cf_cast. Some clients
> might have a reason to use it, just not in these decode functions. It’s the
> same concept as dynamic_objc_cast and dynamic_cast. I think instead we
> should consider removing the assertion from it instead of removing it
> entirely.

I saw the assertion in dynamic_cast<>() as a really useful feature for Debug builds since it would tell you if Release builds were silently dropping the invalid object and returning nullptr.  This is the kind of insight you might want when you can't figure out why code is failing in Release builds because the invalid type is being replaced with nullptr, but you get immediate feedback about the issue when reproducing in a Debug build.

So in my mind, I'd _like_ dynamic_cast-type functions to assert in Debug builds if it was possible to check for an invalid type.  For CF and NS objects, this is possible, but for C/C++ objects it's generally not (although maybe RTTI could do it for C++ if that were enabled).  However, I didn't see this difference in CF/NS vs. C/C++ objects as a reason _not_ to use "dynamic_cast" as the basis for the name.  In fact, I'd see it as a reason to add a Debug assert to check the type to dynamic_objc_cast<>()!

Having said that, I recognize that I'm in the minority here, so I think it's better to get rid of dynamic_cf_cast<>() entirely and to use checked_cf_cast<>() everywhere so we know when we get an unexpected object type every time it happens (as noted above).

[Darin Adler]

This is strange. It’s really not a matter of opinion given that dynamic_cast is built into the language.

We really do have a serious misunderstanding here.

You keep saying *invalid* object. But dynamic_cast is *not* for cases where an object of another type is invalid. It’s for when there is more than one valid type and we are checking for one of the valid types.

That’s why we added checked_cast. It’s for cases where the type has to be a certain one and if it’s not then it’s a programming error or a security vulnerability. It’s much better for those security checks.

But dynamic_cast is not like that. This is not a matter of opinion! It’s a fact of the C++ language design.

You have said that if the object this the wrong type then it is "dropped", and I don’t know where that idea comes from. If the object has the wrong type, then the cast returns nullptr to indicate that. Nothing is "dropped".

We should not use "dynamic cast" as terminology for a runtime-checked cast where we know what type something should be and it’s a security or correctness mistake if it’s the wrong type. I think the name checked_cast is good for casts like that, and we should keep using it.

You said something about being in the minority and I don’t think that’s right. I *agree* that we should use checked casts instead of dynamic casts wherever appropriate. That’s good for security.

But we should use a dynamic cast where it is what we want to do; there is no "silently dropping" concern because it will be doing the right thing, not doing the wrong thing silently!

Here’s where this leads us:

>>> I think we should add *checked_objc_cast* and use it to replace any cases of dynamic_objc_cast where we don’t really want a dynamic cast. Where instead we need a runtime-checked type conversion. But please be careful because not every case of dynamic_objc_cast should be replaced by checked_objc_cast.

For a good example of where we must *not* make that kind change, and of why we might need dynamic_cast some times, look in the ObjCObjectGraph.mm source file at functions like typeFromObject.

The reason I want to keep dynamic_cf_cast around is that I would like to be able to use it for things like that.

>>> Also, we should search for CFGetTypeID and consider changing clients to use *either* checked_cf_cast or dynamic_cf_cast, depending on what they are intending to do. This will tie the type check together with the typecast better than what we have today.

But we should be *careful* deploying checked_cf_cast and checked_objc_cast because they will make the code brittle. It’s risky to add release assertions; they can protect us from security bugs, but they can create crashing bugs, too!

An example where dyanamic_cf_cast might be what we want is populateSetting in WebInspectorClientCF.cpp. Another could be in ArgumentCodersCF.cpp.

[David Kilzer (:ddkilzer)]

Instead of removing WTF::dynamic_cf_cast<>, we need to make sure existing uses have a nullptr check on the returned value (to handle failures), or to use WTF::checked_cf_cast<> instead.

See:
Bug 221428: WTF::dynamic_cf_cast<> should not assert in Debug builds
<https://bugs.webkit.org/show_bug.cgi?id=221428>

Closing this as WONTFIX (NTBF).