[webkit-reviews] review granted: [Bug 61334] Should shim a few SecKeychainItem* methods on SnowLeopard : [Attachment 94671] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 61334: Should shim a few SecKeychainItem* methods on SnowLeopard
https://bugs.webkit.org/show_bug.cgi?id=61334

Attachment 94671: Patch v1
https://bugs.webkit.org/attachment.cgi?id=94671&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.