You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by cl...@apache.org on 2017/10/19 22:22:11 UTC
qpid-proton git commit: PROTON-1620: Windows schannel thread safety
for use with proactor
Repository: qpid-proton
Updated Branches:
refs/heads/master 686a400c9 -> 63b852829
PROTON-1620: Windows schannel thread safety for use with proactor
Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/63b85282
Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/63b85282
Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/63b85282
Branch: refs/heads/master
Commit: 63b8528294d9431f46fc61b0b515c45b70645c4c
Parents: 686a400
Author: Clifford Jansen <cl...@apache.org>
Authored: Thu Oct 19 15:22:42 2017 -0700
Committer: Clifford Jansen <cl...@apache.org>
Committed: Thu Oct 19 15:22:42 2017 -0700
----------------------------------------------------------------------
proton-c/src/ssl/schannel.c | 144 ++++++++++++++++++++++++++++++---------
1 file changed, 110 insertions(+), 34 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/63b85282/proton-c/src/ssl/schannel.c
----------------------------------------------------------------------
diff --git a/proton-c/src/ssl/schannel.c b/proton-c/src/ssl/schannel.c
index 763cae5..788b52d 100644
--- a/proton-c/src/ssl/schannel.c
+++ b/proton-c/src/ssl/schannel.c
@@ -61,6 +61,25 @@ static void ssl_log(pn_transport_t *transport, const char *fmt, ...);
static void ssl_log_error_status(HRESULT status, const char *fmt, ...);
static HCERTSTORE open_cert_db(const char *store_name, const char *passwd, int *error);
+// Thread support. Some SChannel objects are shared or ref-counted.
+// Consistent with the rest of Proton, we assume a connection (and
+// therefore its pn_ssl_t) will not be accessed concurrently by
+// multiple threads.
+class csguard {
+ public:
+ csguard(CRITICAL_SECTION *cs) : cs_(cs), set_(true) { EnterCriticalSection(cs_); }
+ ~csguard() { if (set_) LeaveCriticalSection(cs_); }
+ void release() {
+ if (set_) {
+ set_ = false;
+ LeaveCriticalSection(cs_);
+ }
+ }
+ private:
+ LPCRITICAL_SECTION cs_;
+ bool set_;
+};
+
/*
* win_credential_t: SChannel context that must accompany TLS connections.
*
@@ -73,6 +92,7 @@ static HCERTSTORE open_cert_db(const char *store_name, const char *passwd, int *
* Ref counted by parent ssl_domain_t and each derived connection.
*/
struct win_credential_t {
+ CRITICAL_SECTION cslock;
pn_ssl_mode_t mode;
PCCERT_CONTEXT cert_context; // Particulars of the certificate (if any)
CredHandle cred_handle; // Bound session parameters, certificate, CAs, verification_mode
@@ -88,6 +108,7 @@ struct win_credential_t {
static void win_credential_initialize(void *object)
{
win_credential_t *c = (win_credential_t *) object;
+ InitializeCriticalSectionAndSpinCount(&c->cslock, 4000);
SecInvalidateHandle(&c->cred_handle);
c->cert_context = 0;
c->trust_store = 0;
@@ -106,6 +127,7 @@ static void win_credential_finalize(void *object)
CertCloseStore(c->trust_store, 0);
if (c->server_CA_certs)
CertCloseStore(c->server_CA_certs, 0);
+ DeleteCriticalSection(&c->cslock);
free(c->trust_store_name);
}
@@ -115,9 +137,28 @@ static win_credential_t *win_credential(pn_ssl_mode_t m)
static const pn_class_t clazz = PN_CLASS(win_credential);
win_credential_t *c = (win_credential_t *) pn_class_new(&clazz, sizeof(win_credential_t));
c->mode = m;
+ csguard g(&c->cslock);
+ pn_incref(c); // See next comment regarding refcounting and locks
return c;
}
+// Hack strategy for Proton object refcounting. Just hold the lock for incref.
+// Use the next two functions for decref, one with, the other without the lock.
+// The refcount is artificially bumped by one in win_credential() so that we
+// can use refcount == 1 to actually delete (by calling decref one last time).
+static bool win_credential_decref(win_credential_t *c)
+{
+ // Call with lock held. Caller MUST call win_credential_delete if this returns true.
+ return pn_decref(c) == 1;
+}
+
+static void win_credential_delete(win_credential_t *c)
+{
+ // Call without lock held.
+ assert(pn_refcount(c) == 1);
+ pn_decref(c);
+}
+
static int win_credential_load_cert(win_credential_t *cred, const char *store_name, const char *cert_name, const char *passwd)
{
if (!store_name)
@@ -174,6 +215,7 @@ static int win_credential_load_cert(win_credential_t *cred, const char *store_na
}
+// call with win_credential lock held
static CredHandle win_credential_cred_handle(win_credential_t *cred, pn_ssl_verify_mode_t verify_mode,
const char *session_id, SECURITY_STATUS *status)
{
@@ -227,6 +269,7 @@ typedef enum { UNKNOWN_CONNECTION, SSL_CONNECTION, CLEAR_CONNECTION } connection
typedef struct pn_ssl_session_t pn_ssl_session_t;
struct pn_ssl_domain_t {
+ CRITICAL_SECTION cslock;
int ref_count;
pn_ssl_mode_t mode;
bool has_ca_db; // true when CA database configured
@@ -420,6 +463,8 @@ pn_ssl_domain_t *pn_ssl_domain( pn_ssl_mode_t mode )
pn_ssl_domain_t *domain = (pn_ssl_domain_t *) calloc(1, sizeof(pn_ssl_domain_t));
if (!domain) return NULL;
+ InitializeCriticalSectionAndSpinCount(&domain->cslock, 4000);
+ csguard(&domain->cslock);
domain->ref_count = 1;
domain->mode = mode;
switch(mode) {
@@ -436,14 +481,24 @@ pn_ssl_domain_t *pn_ssl_domain( pn_ssl_mode_t mode )
return domain;
}
+// call with no locks
void pn_ssl_domain_free( pn_ssl_domain_t *domain )
{
if (!domain) return;
-
- if (--domain->ref_count == 0) {
- pn_decref(domain->cred);
- free(domain);
+ {
+ csguard g(&domain->cslock);
+ if (--domain->ref_count)
+ return;
+ }
+ {
+ csguard g2(&domain->cred->cslock);
+ if (win_credential_decref(domain->cred)) {
+ g2.release();
+ win_credential_delete(domain->cred);
+ }
}
+ DeleteCriticalSection(&domain->cslock);
+ free(domain);
}
@@ -453,10 +508,15 @@ int pn_ssl_domain_set_credentials( pn_ssl_domain_t *domain,
const char *password)
{
if (!domain) return -1;
+ csguard g(&domain->cslock);
+ csguard g2(&domain->cred->cslock);
if (win_credential_has_certificate(domain->cred)) {
// Need a new win_credential_t to hold new certificate
- pn_decref(domain->cred);
+ if (win_credential_decref(domain->cred)) {
+ g2.release();
+ win_credential_delete(domain->cred);
+ }
domain->cred = win_credential(domain->mode);
if (!domain->cred)
return -1;
@@ -469,6 +529,7 @@ int pn_ssl_domain_set_trusted_ca_db(pn_ssl_domain_t *domain,
const char *certificate_db)
{
if (!domain || !certificate_db) return -1;
+ csguard g(&domain->cslock);
int ec = 0;
HCERTSTORE store = open_cert_db(certificate_db, NULL, &ec);
@@ -476,14 +537,21 @@ int pn_ssl_domain_set_trusted_ca_db(pn_ssl_domain_t *domain,
return ec;
if (domain->has_ca_db) {
+ csguard g2(&domain->cred->cslock);
win_credential_t *new_cred = win_credential(domain->mode);
- if (!new_cred)
+ if (!new_cred) {
+ CertCloseStore(store, 0);
return -1;
+ }
new_cred->cert_context = CertDuplicateCertificateContext(domain->cred->cert_context);
- pn_decref(domain->cred);
+ if (win_credential_decref(domain->cred)) {
+ g2.release();
+ win_credential_delete(domain->cred);
+ }
domain->cred = new_cred;
}
+ csguard g3(&domain->cred->cslock);
domain->cred->trust_store = store;
domain->cred->trust_store_name = pn_strdup(certificate_db);
domain->has_ca_db = true;
@@ -496,6 +564,9 @@ int pn_ssl_domain_set_peer_authentication(pn_ssl_domain_t *domain,
const char *trusted_CAs)
{
if (!domain) return -1;
+ csguard g(&domain->cslock);
+ csguard g2(&domain->cred->cslock);
+
if (!domain->has_ca_db && (mode == PN_SSL_VERIFY_PEER || mode == PN_SSL_VERIFY_PEER_NAME)) {
ssl_log_error("Error: cannot verify peer without a trusted CA configured.\n"
" Use pn_ssl_domain_set_trusted_ca_db()\n");
@@ -520,29 +591,11 @@ int pn_ssl_domain_set_peer_authentication(pn_ssl_domain_t *domain,
}
int ec = 0;
if (!strcmp(trusted_CAs, domain->cred->trust_store_name)) {
+ changed = true;
store = open_cert_db(trusted_CAs, NULL, &ec);
if (!store)
return ec;
- } else {
- store = CertDuplicateStore(domain->cred->trust_store);
- }
-
- if (domain->cred->server_CA_certs) {
- // Already have one
- changed = true;
- win_credential_t *new_cred = win_credential(domain->mode);
- if (!new_cred) {
- CertCloseStore(store, 0);
- return -1;
- }
- new_cred->cert_context = CertDuplicateCertificateContext(domain->cred->cert_context);
- new_cred->trust_store = CertDuplicateStore(domain->cred->trust_store);
- new_cred->trust_store_name = pn_strdup(domain->cred->trust_store_name);
- pn_decref(domain->cred);
- domain->cred = new_cred;
}
-
- domain->cred->server_CA_certs = store;
}
break;
@@ -557,18 +610,22 @@ int pn_ssl_domain_set_peer_authentication(pn_ssl_domain_t *domain,
if (changed) {
win_credential_t *new_cred = win_credential(domain->mode);
if (!new_cred) {
- CertCloseStore(store, 0);
+ if (store)
+ CertCloseStore(store, 0);
return -1;
}
new_cred->cert_context = CertDuplicateCertificateContext(domain->cred->cert_context);
new_cred->trust_store = CertDuplicateStore(domain->cred->trust_store);
new_cred->trust_store_name = pn_strdup(domain->cred->trust_store_name);
- pn_decref(domain->cred);
+ if (win_credential_decref(domain->cred)) {
+ g2.release();
+ win_credential_delete(domain->cred);
+ }
domain->cred = new_cred;
+ domain->cred->server_CA_certs = store;
}
domain->verify_mode = mode;
- domain->cred->server_CA_certs = store;
return 0;
}
@@ -617,6 +674,8 @@ int pn_ssl_init(pn_ssl_t *ssl0, pn_ssl_domain_t *domain, const char *session_id)
if (!ssl || !domain || ssl->domain) return -1;
if (ssl->state != CREATED) return -1;
+ csguard g(&domain->cslock);
+ csguard g2(&domain->cred->cslock);
ssl->domain = domain;
domain->ref_count++;
if (session_id && domain->mode == PN_SSL_MODE_CLIENT)
@@ -719,6 +778,8 @@ void pn_ssl_free( pn_transport_t *transport)
if (!ssl) return;
ssl_log( transport, "SSL socket freed.\n" );
// clean up Windows per TLS session data before releasing the domain count
+ csguard g(&ssl->domain->cslock);
+ csguard g2(&ssl->cred->cslock);
if (SecIsValidHandle(&ssl->ctxt_handle))
DeleteSecurityContext(&ssl->ctxt_handle);
if (ssl->cred) {
@@ -727,10 +788,16 @@ void pn_ssl_free( pn_transport_t *transport)
if (SecIsValidHandle(&ssl->cred_handle))
FreeCredentialsHandle(&ssl->cred_handle);
}
- pn_decref(ssl->cred);
+ if (win_credential_decref(ssl->cred)) {
+ g2.release();
+ win_credential_delete(ssl->cred);
+ }
}
- if (ssl->domain) pn_ssl_domain_free(ssl->domain);
+ g2.release();
+ g.release();
+ pn_ssl_domain_free(ssl->domain);
+
if (ssl->session_id) free((void *)ssl->session_id);
if (ssl->peer_hostname) free((void *)ssl->peer_hostname);
if (ssl->sc_inbuf) free((void *)ssl->sc_inbuf);
@@ -998,6 +1065,7 @@ static void client_handshake_init(pn_transport_t *transport)
send_buff_desc.ulVersion = SECBUFFER_VERSION;
send_buff_desc.cBuffers = 2;
send_buff_desc.pBuffers = send_buffs;
+ csguard g(&ssl->cred->cslock);
SECURITY_STATUS status = InitializeSecurityContext(&ssl->cred_handle,
NULL, host, ctxt_requested, 0, 0, NULL, 0,
&ssl->ctxt_handle, &send_buff_desc,
@@ -1051,10 +1119,15 @@ static void client_handshake( pn_transport_t* transport) {
send_buff_desc.cBuffers = 2;
send_buff_desc.pBuffers = send_buffs;
- SECURITY_STATUS status = InitializeSecurityContext(&ssl->cred_handle,
+ SECURITY_STATUS status;
+ {
+ csguard g(&ssl->cred->cslock);
+ status = InitializeSecurityContext(&ssl->cred_handle,
&ssl->ctxt_handle, host, ctxt_requested, 0, 0,
&token_buff_desc, 0, NULL, &send_buff_desc,
&ctxt_attrs, NULL);
+ }
+
switch (status) {
case SEC_E_INCOMPLETE_MESSAGE:
// Not enough - get more data from the server then try again.
@@ -1210,10 +1283,13 @@ static void server_handshake(pn_transport_t* transport)
send_buff_desc.pBuffers = send_buffs;
PCtxtHandle ctxt_handle_ptr = (SecIsValidHandle(&ssl->ctxt_handle)) ? &ssl->ctxt_handle : 0;
- SECURITY_STATUS status = AcceptSecurityContext(&ssl->cred_handle, ctxt_handle_ptr,
+ SECURITY_STATUS status;
+ {
+ csguard g(&ssl->cred->cslock);
+ status = AcceptSecurityContext(&ssl->cred_handle, ctxt_handle_ptr,
&token_buff_desc, ctxt_requested, 0, &ssl->ctxt_handle,
&send_buff_desc, &ctxt_attrs, NULL);
-
+ }
bool outbound_token = false;
switch(status) {
case SEC_E_INCOMPLETE_MESSAGE:
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org