Bug 61334 – Should shim a few SecKeychainItem* methods on SnowLeopard

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Should shim a few SecKeychainItem* methods on SnowLeopard

In radar as <rdar://problem/9434311>

[Brady Eidson]

Created attachment 94536 [details]
Refactoring-prep v1 - Move the current SecItem shims into their own implementation file for non-SnowLeopard builds.

[Sam Weinig]

Comment on attachment 94536 [details]
Refactoring-prep v1 - Move the current SecItem shims into their own implementation file for non-SnowLeopard builds.

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

> Source/WebKit2/WebProcess/mac/WebProcessSecItemShim.mm:41
> +static bool UseSecItemShim = false;

This should not start with a capital letter. There is no need to initialize it to false.

> Source/WebKit2/WebProcess/mac/WebProcessSecItemShim.mm:48
> +struct SecItemAPIContext
> +{

{ should be on previous line.

[Brady Eidson]

Created attachment 94546 [details]
Refactoring-prep v2 - Keep the shim callbacks in the WebKit2 framework, and the shims in the WebProcessShim

Had to rework this a bit because of the WebProcess / WebProcessShim divide.

Totally moved that bracket, though!

[WebKit Commit Bot]

Comment on attachment 94546 [details]
Refactoring-prep v2 - Keep the shim callbacks in the WebKit2 framework, and the shims in the WebProcessShim

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

[Brady Eidson]

Created attachment 94671 [details]
Patch v1

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/Shared/mac/KeychainAttribute.cpp:45:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit2/Shared/mac/SecKeychainItemRequestData.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/Shared/mac/SecKeychainItemResponseData.h:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/Shared/mac/KeychainAttribute.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 16 files


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

[Darin Adler]

Comment on attachment 94671 [details]
Patch v1

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

I’m concerned about the global data structures that should be per-web-process instead. Otherwise looks fine.

> Source/WebKit2/Shared/mac/KeychainAttribute.cpp:45
> +    data.adoptCF(CFDataCreate(NULL, (UInt8*)secKeychainAttribute.data, secKeychainAttribute.length));

Why is a cast needed here? Why a C-style cast? Why NULL instead of 0?

> Source/WebKit2/Shared/mac/KeychainAttribute.cpp:73
> +    attribute.tag = attributeTag;

Why do this at the bottom of the function instead of just after decoding it?

> Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp:104
> +    m_attributeList = (SecKeychainAttributeList*)calloc(1, sizeof(SecKeychainAttributeList));

How about a C++ style cast instead of C-style?

How about using new instead of malloc so you could use OwnPtr, making it harder to make lifetime mistakes? It’s easy to zero out the object afterward. Nothing magic about calloc in that respect.

> Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp:106
> +    m_attributeList->attr = (SecKeychainAttribute*)calloc(m_attributeList->count, sizeof(SecKeychainAttribute));

How about a C++ style cast instead of C-style?

How about using new instead of malloc so you could use OwnArrayPtr, making it harder to make lifetime mistakes?

> Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp:114
> +        m_attributeList->attr[i].data = (void*)CFDataGetBytePtr(m_attributes[i].data.get());

How about a C++ style cast instead of C-style?

> Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp:161
> +    secKeychainItemRequestData.m_itemClass = (SecItemClass)itemClass;

How about a C++ cast instead of a C cast?

> Source/WebKit2/Shared/mac/SecKeychainItemRequestData.h:42
> +class SecKeychainItemRequestData {

This should be noncopyable. That would happen automatically if you used OwnPtr or OwnArrayPtr for any of the data members.

> Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:90
> +    RetainPtr<CFDataRef> data(AdoptCF, CFDataCreate(0, (const UInt8*)outData, length));

How about a C++ cast instead of a C cast?

> Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:101
> +    OSStatus resultCode = SecKeychainItemCreateFromContent(request.itemClass(), request.attributeList(), request.length(), request.data(), 
> +                                                           NULL, NULL, &keychainItem);

Not the way we usually line things up.

> Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:110
> +    response = SecKeychainItemResponseData(resultCode);

No need for the explicit conversion here, unless you make this constructor explicit above.

> Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm:41
> +static HashSet<SecKeychainAttributeList*>& shimManagedAttributeLists()

I think this needs to be a per-web-process data structure, not a single global one.

> Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm:83
> +        CFDataGetBytes(cfData, CFRangeMake(0, length), (UInt8*)attrList->attr[i].data);

How about a C++ style cast here?

> Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm:92
> +// Methods to allow the shim to manage memory for its own KeychainItem content data.
> +static HashSet<void*>& shimManagedKeychainItemContents()
> +{
> +    DEFINE_STATIC_LOCAL(HashSet<void*>, set, ());
> +    return set;
> +}

I think this needs to be a per-web-process data structure, not a single global one.

> Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm:197
> +        ASSERT_NOT_REACHED();

Seems inappropriate to assert here. This could happen if the other process died, for example.

> Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm:296
> +    WebProcessKeychainItemShimInitializeFunc func = reinterpret_cast<WebProcessKeychainItemShimInitializeFunc>(dlsym(RTLD_DEFAULT, "WebKitWebProcessKeychainItemShimInitialize"));
> +    func(callbacks);

I’d prefer that you not abbreviate to fund here.

[Brady Eidson]

(In reply to comment #7)
> (From update of attachment 94671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94671&action=review
> 
> I’m concerned about the global data structures that should be per-web-process instead. Otherwise looks fine.

Since it's possibly relevant in the future, I'll fix that up now.

> > Source/WebKit2/Shared/mac/KeychainAttribute.cpp:45
> > +    data.adoptCF(CFDataCreate(NULL, (UInt8*)secKeychainAttribute.data, secKeychainAttribute.length));
> 
> Why is a cast needed here? Why a C-style cast? Why NULL instead of 0?

Cast is needed because strict warnings make the compiler choke on "void*" -> "UInt8*".  I can make it c++-style, as with all the others.

NULL because I thought our style was to use NULL in C-API methods.  I guess I was wrong.
> 
> > Source/WebKit2/Shared/mac/KeychainAttribute.cpp:73
> > +    attribute.tag = attributeTag;
> 
> Why do this at the bottom of the function instead of just after decoding it?

WIll change.

> 
> > Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp:104
> > +    m_attributeList = (SecKeychainAttributeList*)calloc(1, sizeof(SecKeychainAttributeList));
> 
> How about a C++ style cast instead of C-style?
> 
> How about using new instead of malloc so you could use OwnPtr, making it harder to make lifetime mistakes? It’s easy to zero out the object afterward. Nothing magic about calloc in that respect.

Reasonable.

> 
> > Source/WebKit2/Shared/mac/SecKeychainItemRequestData.cpp:106
> > +    m_attributeList->attr = (SecKeychainAttribute*)calloc(m_attributeList->count, sizeof(SecKeychainAttribute));
> 
> How about a C++ style cast instead of C-style?
> 
> How about using new instead of malloc so you could use OwnArrayPtr, making it harder to make lifetime mistakes?
> 

Reasonable

> 
> > Source/WebKit2/Shared/mac/SecKeychainItemRequestData.h:42
> > +class SecKeychainItemRequestData {
> 
> This should be noncopyable. That would happen automatically if you used OwnPtr or OwnArrayPtr for any of the data members.

Okay!

> 
> > Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:101
> > +    OSStatus resultCode = SecKeychainItemCreateFromContent(request.itemClass(), request.attributeList(), request.length(), request.data(), 
> > +                                                           NULL, NULL, &keychainItem);
> 
> Not the way we usually line things up.

Didn't do that on purpose - will fix.

> 
> > Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:110
> > +    response = SecKeychainItemResponseData(resultCode);
> 
> No need for the explicit conversion here, unless you make this constructor explicit above.


> 
> > Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm:197
> > +        ASSERT_NOT_REACHED();
> 
> Seems inappropriate to assert here. This could happen if the other process died, for example.

If the UIProcess dies, we don't care about the sanctity of the WebProcess anymore.  The ASSERT helped me catch encode/decode mistakes, and I'd like to leave it to catch future ones if this code changes much.

> 
> > Source/WebKit2/WebProcess/mac/KeychainItemShimMethods.mm:296
> > +    WebProcessKeychainItemShimInitializeFunc func = reinterpret_cast<WebProcessKeychainItemShimInitializeFunc>(dlsym(RTLD_DEFAULT, "WebKitWebProcessKeychainItemShimInitialize"));
> > +    func(callbacks);
> 
> I’d prefer that you not abbreviate to fund here.

Okay!

[Brady Eidson]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 94671 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=94671&action=review
> > 
> > I’m concerned about the global data structures that should be per-web-process instead. Otherwise looks fine.
> 
> Since it's possibly relevant in the future, I'll fix that up now.
>

Oh duh, this is already in the WebProcess.  :)

[Brady Eidson]

First patch landed in r87153 and r87154.

Second patch landed in r87221.