Bug 177895 – Re-enable -Wcast-qual in WebCore for Apple ports

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

For Bug 177893, we disabled -Wcast-qual in WebCore for Apple ports.

This bug tracks re-enabling it by introducing a safe (and readable) way to cast const CFTypeRef pointers to non-const CFTypeRef pointers (sometimes of a different type).

[Radar WebKit Bug Importer]

<rdar://problem/34960830>

[David Kilzer (:ddkilzer)]

Created attachment 330270 [details]
Patch v1

[EWS Watchlist]

Attachment 330270 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330271 [details]
Patch v2

[EWS Watchlist]

Attachment 330271 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330273 [details]
Patch v3

[EWS Watchlist]

Attachment 330273 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330276 [details]
Patch v4

[EWS Watchlist]

Attachment 330276 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330282 [details]
Patch v5

[EWS Watchlist]

Attachment 330282 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330283 [details]
Patch v6

[EWS Watchlist]

Attachment 330283 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:63:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:110:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:111:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330284 [details]
Patch v7

[EWS Watchlist]

Attachment 330284 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:63:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:110:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:111:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330286 [details]
Patch v8

[EWS Watchlist]

Attachment 330286 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:63:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:110:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:111:  dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 330286 [details]
Patch v8

Attachment 330286 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5885909

New failing tests:
fast/mediastream/MediaStream-MediaElement-setObject-null.html

[EWS Watchlist]

Created attachment 330292 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[David Kilzer (:ddkilzer)]

Created attachment 330304 [details]
Patch v9

[EWS Watchlist]

Attachment 330304 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330306 [details]
Patch v10

[EWS Watchlist]

Attachment 330306 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 330309 [details]
Patch v11

[EWS Watchlist]

Attachment 330309 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Comment on attachment 330309 [details]
Patch v11

View in context: https://bugs.webkit.org/attachment.cgi?id=330309&action=review

> Source/WTF/ChangeLog:40
> +        (WTF::checked_cf_const_cast): This behaves the same way as
> +        WTF::checked_cf_cast does now, but works with CFTypes that are
> +        not defined as const pointers, including CFMutable types and
> +        other CFTypes defined outside of CoreFoundation.framework.  We
> +        also add built-in support for CFMutable types in the header by
> +        defining traits for them.

Note that I added WTF::checked_cf_const_cast<> so that non-const CFTypes (the CFMutable* types) had a separate method to use.

This makes it harder (in theory) to incorrectly cast to CFArray from CFMutableArray since you must use checked_cf_cast<CFArrayRef>() and checked_cf_const_cast<CFMutableArrayRef>().

In practice, I'm not sure how much this difference matters.  I can just add the const_cast<CF_BRIDGED_TYPE(ID) void*>() to WTF::checked_cf_cast<> and the compiler wouldn't complain about the 'const' being removed and then re-added.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #26)
> Comment on attachment 330309 [details]
> Patch v11
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330309&action=review
> 
> > Source/WTF/ChangeLog:40
> > +        (WTF::checked_cf_const_cast): This behaves the same way as
> > +        WTF::checked_cf_cast does now, but works with CFTypes that are
> > +        not defined as const pointers, including CFMutable types and
> > +        other CFTypes defined outside of CoreFoundation.framework.  We
> > +        also add built-in support for CFMutable types in the header by
> > +        defining traits for them.
> 
> Note that I added WTF::checked_cf_const_cast<> so that non-const CFTypes
> (the CFMutable* types) had a separate method to use.
> 
> This makes it harder (in theory) to incorrectly cast to CFArray from
> CFMutableArray since you must use checked_cf_cast<CFArrayRef>() and
> checked_cf_const_cast<CFMutableArrayRef>().
> 
> In practice, I'm not sure how much this difference matters.  I can just add
> the const_cast<CF_BRIDGED_TYPE(ID) void*>() to WTF::checked_cf_cast<> and
> the compiler wouldn't complain about the 'const' being removed and then
> re-added.

And I can check that CFMutable* objects are actually mutable using something like this (which would be a Debug assertion to start with since there is no security implication):

inline bool CFArrayIsMutable(CFArrayRef array)
{
    return [(id)array isMemberOfClass:[NSMutableArray class]];
}

[David Kilzer (:ddkilzer)]

Created attachment 330366 [details]
Patch v12

[EWS Watchlist]

Attachment 330366 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:84:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #27)
> (In reply to David Kilzer (:ddkilzer) from comment #26)
> > Comment on attachment 330309 [details]
> > Patch v11
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=330309&action=review
> > 
> > > Source/WTF/ChangeLog:40
> > > +        (WTF::checked_cf_const_cast): This behaves the same way as
> > > +        WTF::checked_cf_cast does now, but works with CFTypes that are
> > > +        not defined as const pointers, including CFMutable types and
> > > +        other CFTypes defined outside of CoreFoundation.framework.  We
> > > +        also add built-in support for CFMutable types in the header by
> > > +        defining traits for them.
> > 
> > Note that I added WTF::checked_cf_const_cast<> so that non-const CFTypes
> > (the CFMutable* types) had a separate method to use.
> > 
> > This makes it harder (in theory) to incorrectly cast to CFArray from
> > CFMutableArray since you must use checked_cf_cast<CFArrayRef>() and
> > checked_cf_const_cast<CFMutableArrayRef>().
> > 
> > In practice, I'm not sure how much this difference matters.  I can just add
> > the const_cast<CF_BRIDGED_TYPE(ID) void*>() to WTF::checked_cf_cast<> and
> > the compiler wouldn't complain about the 'const' being removed and then
> > re-added.
> 
> And I can check that CFMutable* objects are actually mutable using something
> like this (which would be a Debug assertion to start with since there is no
> security implication):
> 
> inline bool CFArrayIsMutable(CFArrayRef array)
> {
>     return [(id)array isMemberOfClass:[NSMutableArray class]];
> }

Actually this won't work, so will need to make more changes.

[David Kilzer (:ddkilzer)]

Created attachment 330394 [details]
Patch v13

[EWS Watchlist]

Attachment 330394 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:70:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #31)
> Created attachment 330394 [details]
> Patch v13

Okay, I'm done iterating on this.  It's ready for review.

[Joseph Pecoraro]

Comment on attachment 330394 [details]
Patch v13

View in context: https://bugs.webkit.org/attachment.cgi?id=330394&action=review

Looks good to me. I didn't really understand the RetainPtr changes though so I'll let someone else review.

> Source/WTF/ChangeLog:30
> +        Release buidls, as well as other "checked" functions.

Typo: "buidls" => "builds"

> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:47
>      attributesDictionary = adoptCF(CFDictionaryCreateMutable(kCFAllocatorDefault, 4, &kCFCopyStringDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> -    CFMutableDictionaryRef mutableAttributes = (CFMutableDictionaryRef)attributesDictionary.get();
> +    CFMutableDictionaryRef mutableAttributes = checked_cf_cast<CFMutableDictionaryRef>(attributesDictionary.get());

Here I'd probably just switch to using `attributesDictionary.get()` locally instead of `mutableAttributes`. This is a local we just created on line 46, we shouldn't need to check its type.

> Source/WebCore/testing/cocoa/WebArchiveDumpSupport.mm:223
> -    RetainPtr<CFMutableDictionaryRef> propertyList = adoptCF((CFMutableDictionaryRef)CFPropertyListCreateWithData(kCFAllocatorDefault, webArchiveData, kCFPropertyListMutableContainersAndLeaves, &format, &error));
> +    RetainPtr<CFMutableDictionaryRef> propertyList = adoptCF(checked_cf_cast<CFMutableDictionaryRef>(CFPropertyListCreateWithData(kCFAllocatorDefault, webArchiveData, kCFPropertyListMutableContainersAndLeaves, &format, &error)));

For these WebArchive cases, I think that if we got an unexpected type that we would want to bail with an error message instead of crashing (inside a cast check). That said the checked casts are certainly better then previous behavior.

[David Kilzer (:ddkilzer)]

Comment on attachment 330394 [details]
Patch v13

View in context: https://bugs.webkit.org/attachment.cgi?id=330394&action=review

>> Source/WTF/ChangeLog:30
>> +        Release buidls, as well as other "checked" functions.
> 
> Typo: "buidls" => "builds"

Fixed.  Thanks for spotting this!

>> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:47
>> +    CFMutableDictionaryRef mutableAttributes = checked_cf_cast<CFMutableDictionaryRef>(attributesDictionary.get());
> 
> Here I'd probably just switch to using `attributesDictionary.get()` locally instead of `mutableAttributes`. This is a local we just created on line 46, we shouldn't need to check its type.

Done!  Had to change the type of m_kernedCFStringAttributes and m_nonKernedCFStringAttributes in Font.h from RetainPtr<CFDictionaryRef> to RetainPtr<CFMutableDictionaryRef>, but that's more truthy anyway.

>> Source/WebCore/testing/cocoa/WebArchiveDumpSupport.mm:223
>> +    RetainPtr<CFMutableDictionaryRef> propertyList = adoptCF(checked_cf_cast<CFMutableDictionaryRef>(CFPropertyListCreateWithData(kCFAllocatorDefault, webArchiveData, kCFPropertyListMutableContainersAndLeaves, &format, &error)));
> 
> For these WebArchive cases, I think that if we got an unexpected type that we would want to bail with an error message instead of crashing (inside a cast check). That said the checked casts are certainly better then previous behavior.

At best we could output the CFTypeID of the incorrectly typed object.  If it's a CoreFoundation type, then we might be able to give the name of the object type in the log output, but for CFTypes like DDResultRef (as a random example that probably would never happen here), I think the CFTypeID is assigned at runtime, so there's no way we could exhaustively map a CFTypeID to a string name at runtime.  So I'm not sure that the CFTypeID by itself would be all that useful in the crash output (again unless it's a native CoreFoundation type--although I didn't check if those had static CFTypeIDs or not).

[David Kilzer (:ddkilzer)]

(In reply to Joseph Pecoraro from comment #34)
> Comment on attachment 330394 [details]
> Patch v13
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330394&action=review
> 
> Looks good to me. I didn't really understand the RetainPtr changes though so
> I'll let someone else review.

Internally, RetainPtr<> stores a pointer to the object it references using CFTypeRef:

    typedef CFTypeRef StorageType;

And CFTypeRef is defined as:

    typedef const CF_BRIDGED_TYPE(id) void * CFTypeRef;

The reason the const_cast<> is needed here is that some CF objects (like CFMutable*Ref types and most CF types defined outside of CoreFoundation.framework) are defined as non-const pointers (compare CFArrayRef to CFMutableArrayRef):

    typedef const struct CF_BRIDGED_TYPE(NSArray) __CFArray * CFArrayRef;
    typedef struct CF_BRIDGED_MUTABLE_TYPE(NSMutableArray) __CFArray * CFMutableArrayRef;

    typedef struct DD_BRIDGED_TYPE(id) __DDResult * DDResultRef;

So when we request the "raw" pointer from a RetainPtr<> (using get() or leakRef()) with a template type that doesn't have a 'const' modifier, we need the const_cast<> to make sure the compiler doesn't emit a -Wcast-qual warning when casting from the internal 'const' (CFTypeRef) pointer to 'non-const' (template type) pointer.

The const_cast<> is not needed for CFTypes with 'const' modifiers, but it does no harm since casting from non-const back to const is okay.

[David Kilzer (:ddkilzer)]

Created attachment 330574 [details]
Patch v14

[EWS Watchlist]

Attachment 330574 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:70:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 330574 [details]
Patch v14

r=me, thanks for the detailed explanation

[WebKit Commit Bot]

Comment on attachment 330574 [details]
Patch v14

Rejecting attachment 330574 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 330574, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
.webkit.org/git/WebKit
   e552102..c1c0711  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 226466 = e55210209101d2e7c86dddd6a26640e11db1ea9d
r226467 = c1c07115837487ce093672773264ad8605be5ef6
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/5947220

[WebKit Commit Bot]

Comment on attachment 330574 [details]
Patch v14

Clearing flags on attachment: 330574

Committed r226483: <https://trac.webkit.org/changeset/226483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.