Mac: Introduce "StaticCast" variants of base::mac::FooCastStrict, which do not typecheck in Release

base/mac/foundation_util.h

Index: base/mac/foundation_util.h
diff --git a/base/mac/foundation_util.h b/base/mac/foundation_util.h
index ee23a17fb16e6c0c5c6856c6cd0ccbd9dd43c120..6505e614ed56b2120a23d8318126a90bc063112f 100644
--- a/base/mac/foundation_util.h
+++ b/base/mac/foundation_util.h
@@ -272,14 +272,16 @@ template<typename T>
 T CFCast(const CFTypeRef& cf_val);
 
 template<typename T>
-T CFCastStrict(const CFTypeRef& cf_val);
+T CFCastStrict(const CFTypeRef& cf_val) {
+  DCHECK(!cf_val || CFCast<T>(cf_val));
+  // Note: CFTypeRef is const void*, but CGColorRef and some others are non-
+  // const. So this may also cast away a const qualifier.
+  return (T)(cf_val);

[Mark Mentovai, 2016/05/05 13:26:31]

So now in release mode, the “strict” variants will NOT return nullptr/nil even
if the type doesn’t match. That seems kind of bad, considering how these are
often used to protect us against type-confusion bugs.

[tapted, 2016/05/06 01:57:09]

currently they may protect against a _crash_ from a type confusion bug, but not
necessarily against other kinds of bugs. I guess this is more true for the ObjC
variant than the CF one, but there returning null/nil isn't really any good if
the caller doesn't check for null. In Release. I would rather have an actual
crash like `uncaught unrecognized selector sent to instance` than have calls on
the `nil` absorbed by the runtime.

Also since these already DCHECK, they kinda go against the spirit of the regime
imposed at
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...
 i.e. "you should not handle DCHECK() failures, even if failure would result in
a crash"

But if changing existing callers is too scary, how would you feel about adding
another variant of these called "CFStaticCast" and "ObjCStaticCast", with this
implementation? Then things with good test coverage can opt for the "Static"
versions and not suffer the runtime penalty of the type checking.

[Mark Mentovai, 2016/05/06 15:34:41]

tapted wrote:
The big problem with type confusion is that you can’t nail down the behavior at
all when you treat a pointer to one type as a pointer to another. It’s totally
dependent on the data. Sometimes it’s a crash, but sometimes it’s some other
unexpected garbage. Sometimes it gets nasty, and in the worst cases, there are
security implications.

Returning nullptr or nil when the type doesn’t match reduces all of this to a
non-issue. Even if the calling code doesn’t handle nullptr or nil, it’s not
going to result in unexpected garbage. You might get a crash, but it’s a clean
crash with no security implications and it’ll be obvious in the post-mortem that
the pointer was 0.

In most instances in Objective-C, it’ll just turn into a no-op. So no, you won’t
see “unrecognized selector,” but you also weren’t guaranteed that anyway,
because the selector you’re calling may have been implemented in both classes.
And this is where type confusion gets really confusing. You enter an unexpected
codepath that seems to do the right thing for a while, and then when things hit
the fan, you’re in too deep to figure out where you actually went off the rails.

But in reality, when you’re slurping stuff out of a dictionary (which is
probably where this is used most commonly), you probably EITHER want to check
that the key was actually in the dictionary (and you’d achieve that with a nil
check) OR you’d just let messages to nil turn into no-ops (which the cast as-is
gives you).
Where are the hot paths that suffer from a performance problem? Most things that
come through here are not hot code, or they probably wouldn’t be using CF or
Objective-C to begin with.

+}
 
 #define CF_CAST_DECL(TypeCF) \
 template<> BASE_EXPORT TypeCF##Ref \
-CFCast<TypeCF##Ref>(const CFTypeRef& cf_val);\
-\
-template<> BASE_EXPORT TypeCF##Ref \
-CFCastStrict<TypeCF##Ref>(const CFTypeRef& cf_val);
+CFCast<TypeCF##Ref>(const CFTypeRef& cf_val);
 
 CF_CAST_DECL(CFArray);
 CF_CAST_DECL(CFBag);
@@ -339,9 +341,8 @@ T* ObjCCast(id objc_val) {
 
 template<typename T>
 T* ObjCCastStrict(id objc_val) {
-  T* rv = ObjCCast<T>(objc_val);
-  DCHECK(objc_val == nil || rv);
-  return rv;
+  DCHECK(!objc_val || ObjCCast<T>(objc_val));
+  return reinterpret_cast<T*>(objc_val);
 }
 
 #endif  // defined(__OBJC__)