New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

darwinssl: fix iOS build and CFArrayRef leak #1173

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@chris-araman
Contributor

chris-araman commented Dec 23, 2016

#1172

  • use SecCertificateCopySubjectSummary on iOS as we do in CopyCertSubject
  • retain the matching SecIdentityRef and release the others with CFRelease(keys_list)
  • move locals into #if CURL_BUILD_MAC_10_7 || CURL_BUILD_IOS block to avoid unused locals warnings on other targets
  • set status = 1 in case SecItemCopyMatching yields an empty set

I haven't been able to independently verify whether client certificate matching works on any version of iOS or macOS, before or after this change. @schweikert, @nickzman, would you be able to help with that? Presumably, this change is no worse for iOS than what we had before #1105.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 23, 2016

@chris-araman, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schweikert, @nickzman and @bagder to be potential reviewers.

mention-bot commented Dec 23, 2016

@chris-araman, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schweikert, @nickzman and @bagder to be potential reviewers.

@schweikert

This comment has been minimized.

Show comment
Hide comment
@schweikert

schweikert Dec 23, 2016

Contributor

Unfortunately I can't test this, because I don't have access to a web server requesting a client-certificate anymore. The patch looks good to me though.

Thanks for this, and sorry for breaking the build on iOS.

Contributor

schweikert commented Dec 23, 2016

Unfortunately I can't test this, because I don't have access to a web server requesting a client-certificate anymore. The patch looks good to me though.

Thanks for this, and sorry for breaking the build on iOS.

@chris-araman

This comment has been minimized.

Show comment
Hide comment
@chris-araman

chris-araman Dec 23, 2016

Contributor

No worries, @schweikert. Thanks for contributing!

Contributor

chris-araman commented Dec 23, 2016

No worries, @schweikert. Thanks for contributing!

@chris-araman chris-araman changed the title from fix iOS build and CFArrayRef leak to darwinssl: fix iOS build and CFArrayRef leak Dec 23, 2016

if(err == noErr) {
#if CURL_BUILD_IOS
common_name = SecCertificateCopySubjectSummary(cert);
#elif CURL_BUILD_MAC_10_7

This comment has been minimized.

@nickzman

nickzman Dec 24, 2016

Collaborator

Shouldn't this just be #else?

@nickzman

nickzman Dec 24, 2016

Collaborator

Shouldn't this just be #else?

This comment has been minimized.

@chris-araman

chris-araman Dec 24, 2016

Contributor

I considered that, but thought this would be clearer, especially if the outer #if ever changes. Feel free to change it, or require that I do.

@chris-araman

chris-araman Dec 24, 2016

Contributor

I considered that, but thought this would be clearer, especially if the outer #if ever changes. Feel free to change it, or require that I do.

@nickzman

Sorry for the late approval. The change looks okay to me, and doesn't break the build. I didn't have time to test this under iOS with a client certificate in the keychain, but in theory, this ought to work.

@chris-araman

This comment has been minimized.

Show comment
Hide comment
@chris-araman

chris-araman Dec 27, 2016

Contributor

No problem. Holiday and such. Thanks!

Contributor

chris-araman commented Dec 27, 2016

No problem. Holiday and such. Thanks!

@bagder bagder closed this in e53f073 Dec 27, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 27, 2016

Member

Thanks, both of you!

Member

bagder commented Dec 27, 2016

Thanks, both of you!

@chris-araman chris-araman deleted the chris-araman:issue-1172 branch Dec 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment