Skip to content

Commit

Permalink
Locking: Make sure all app callbacks are properly locked.
Browse files Browse the repository at this point in the history 
Recursive lock issues happen for some of the app callbacks that then call a
libcoap public API that tries to lock up the CoAP context.

All libcoap callbacks into app code should now be fixed.
  • Loading branch information
@mrdeep1
mrdeep1 committed Jul 11, 2024
1 parent aa46177 commit 53e9333
Show file tree
Hide file tree
Showing 8 changed files with 145 additions and 103 deletions.
29 changes: 17 additions & 12 deletions src/coap_block.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -667,8 +667,9 @@ coap_add_data_large_internal(coap_session_t *session,
assert(pdu);
if (pdu->data) {
coap_log_warn("coap_add_data_large: PDU already contains data\n");
if (release_func)
release_func(session, app_ptr);
if (release_func) {
coap_lock_callback(session->context, release_func(session, app_ptr));
}
return 0;
}

Expand Down Expand Up @@ -865,8 +866,9 @@ coap_add_data_large_internal(coap_session_t *session,
if (!coap_add_data(pdu, rem, &data[block.num * chunk]))
goto fail;
}
if (release_func)
release_func(session, app_ptr);
if (release_func) {
coap_lock_callback(session->context, release_func(session, app_ptr));
}
} else if ((have_block_defined && length > chunk) || (ssize_t)length > avail) {
/* Only add in lg_xmit if more than one block needs to be handled */
size_t rem;
Expand Down Expand Up @@ -1065,16 +1067,17 @@ coap_add_data_large_internal(coap_session_t *session,
if (!coap_add_data(pdu, length, data))
goto fail;

if (release_func)
release_func(session, app_ptr);
if (release_func) {
coap_lock_callback(session->context, release_func(session, app_ptr));
}
}
return 1;

fail:
if (lg_xmit) {
coap_block_delete_lg_xmit(session, lg_xmit);
} else if (release_func) {
release_func(session, app_ptr);
coap_lock_callback(session->context, release_func(session, app_ptr));
}
return 0;
}
Expand Down Expand Up @@ -1109,8 +1112,9 @@ coap_add_data_large_request_lkd(coap_session_t *session,
* E.g. Reliable and CSM not in yet for checking block support
*/
if (coap_client_delay_first(session) == 0) {
if (release_func)
release_func(session, app_ptr);
if (release_func) {
coap_lock_callback(session->context, release_func(session, app_ptr));
}
return 0;
}
return coap_add_data_large_internal(session, NULL, pdu, NULL, NULL, -1, 0,
Expand Down Expand Up @@ -1249,8 +1253,9 @@ coap_add_data_large_response_lkd(coap_resource_t *resource,
return 1;

error:
if (release_func)
release_func(session, app_ptr);
if (release_func) {
coap_lock_callback(session->context, release_func(session, app_ptr));
}
error_released:
#if COAP_ERROR_PHRASE_LENGTH > 0
coap_add_data(response,
Expand Down Expand Up @@ -2334,7 +2339,7 @@ coap_block_delete_lg_xmit(coap_session_t *session,
return;

if (lg_xmit->release_func) {
lg_xmit->release_func(session, lg_xmit->app_ptr);
coap_lock_callback(session->context, lg_xmit->release_func(session, lg_xmit->app_ptr));
}
if (lg_xmit->pdu.token) {
coap_free_type(COAP_PDU_BUF, lg_xmit->pdu.token - lg_xmit->pdu.max_hdr_size);
Expand Down
2 changes: 1 addition & 1 deletion src/coap_cache.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ coap_delete_cache_entry(coap_context_t *ctx, coap_cache_entry_t *cache_entry) {
}
coap_delete_cache_key(cache_entry->cache_key);
if (cache_entry->callback && cache_entry->app_data) {
cache_entry->callback(cache_entry->app_data);
coap_lock_callback(ctx, cache_entry->callback(cache_entry->app_data));
}
coap_free_type(COAP_CACHE_ENTRY, cache_entry);
}
Expand Down
67 changes: 37 additions & 30 deletions src/coap_gnutls.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,10 +593,10 @@ psk_client_callback(gnutls_session_t g_session,

lhint.length = temp.length;
lhint.s = temp.s;
cpsk_info =
setup_data->validate_ih_call_back(&lhint,
c_session,
setup_data->ih_call_back_arg);
coap_lock_callback_ret(cpsk_info, c_session->context,
setup_data->validate_ih_call_back(&lhint,
c_session,
setup_data->ih_call_back_arg));

if (cpsk_info == NULL)
return -1;
Expand Down Expand Up @@ -753,13 +753,15 @@ check_rpk_cert(coap_gnutls_context_t *g_context,
G_CHECK(gnutls_pubkey_export(pcert.pubkey, GNUTLS_X509_FMT_DER, der, &size),
"gnutls_pubkey_export");
gnutls_pcert_deinit(&pcert);
if (!g_context->setup_data.validate_cn_call_back(COAP_DTLS_RPK_CERT_CN,
der,
size,
c_session,
0,
1,
g_context->setup_data.cn_call_back_arg)) {
coap_lock_callback_ret(ret, c_session->context,
g_context->setup_data.validate_cn_call_back(COAP_DTLS_RPK_CERT_CN,
der,
size,
c_session,
0,
1,
g_context->setup_data.cn_call_back_arg));
if (!ret) {
return 0;
}
}
Expand Down Expand Up @@ -910,13 +912,15 @@ cert_verify_gnutls(gnutls_session_t g_session) {
G_CHECK(gnutls_x509_crt_export(cert, GNUTLS_X509_FMT_DER, der, &size),
"gnutls_x509_crt_export");
gnutls_x509_crt_deinit(cert);
if (!g_context->setup_data.validate_cn_call_back(OUTPUT_CERT_NAME,
der,
size,
c_session,
0,
cert_is_trusted,
g_context->setup_data.cn_call_back_arg)) {
coap_lock_callback_ret(ret, c_session->context,
g_context->setup_data.validate_cn_call_back(OUTPUT_CERT_NAME,
der,
size,
c_session,
0,
cert_is_trusted,
g_context->setup_data.cn_call_back_arg));
if (!ret) {
alert = GNUTLS_A_ACCESS_DENIED;
goto fail;
}
Expand Down Expand Up @@ -1528,10 +1532,12 @@ post_client_hello_gnutls_psk(gnutls_session_t g_session) {
/*
* New SNI request
*/
const coap_dtls_spsk_info_t *new_entry =
c_session->context->spsk_setup_data.validate_sni_call_back(name,
c_session,
c_session->context->spsk_setup_data.sni_call_back_arg);
const coap_dtls_spsk_info_t *new_entry;

coap_lock_callback_ret(new_entry, c_session->context,
c_session->context->spsk_setup_data.validate_sni_call_back(name,
c_session,
c_session->context->spsk_setup_data.sni_call_back_arg));
if (!new_entry) {
G_ACTION(gnutls_alert_send(g_session, GNUTLS_AL_FATAL,
GNUTLS_A_UNRECOGNIZED_NAME));
Expand Down Expand Up @@ -1642,9 +1648,11 @@ post_client_hello_gnutls_pki(gnutls_session_t g_session) {
/*
* New SNI request
*/
coap_dtls_key_t *new_entry =
g_context->setup_data.validate_sni_call_back(name,
g_context->setup_data.sni_call_back_arg);
coap_dtls_key_t *new_entry;

coap_lock_callback_ret(new_entry, c_session->context,
g_context->setup_data.validate_sni_call_back(name,
g_context->setup_data.sni_call_back_arg));
if (!new_entry) {
G_ACTION(gnutls_alert_send(g_session, GNUTLS_AL_FATAL,
GNUTLS_A_UNRECOGNIZED_NAME));
Expand All @@ -1660,11 +1668,10 @@ post_client_hello_gnutls_pki(gnutls_session_t g_session) {
g_context->pki_sni_entry_list[i].pki_key = *new_entry;
sni_setup_data = g_context->setup_data;
sni_setup_data.pki_key = *new_entry;
if ((ret = setup_pki_credentials(
&g_context->pki_sni_entry_list[i].pki_credentials,
g_session,
g_context,
&sni_setup_data, COAP_DTLS_ROLE_SERVER)) < 0) {
if ((ret = setup_pki_credentials(&g_context->pki_sni_entry_list[i].pki_credentials,
g_session,
g_context,
&sni_setup_data, COAP_DTLS_ROLE_SERVER)) < 0) {
int keep_ret = ret;
G_ACTION(gnutls_alert_send(g_session, GNUTLS_AL_FATAL,
GNUTLS_A_BAD_CERTIFICATE));
Expand Down
32 changes: 18 additions & 14 deletions src/coap_mbedtls.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,13 +474,17 @@ cert_verify_callback_mbedtls(void *data, mbedtls_x509_crt *crt,
*flags &= ~MBEDTLS_X509_BADCERT_CN_MISMATCH;
}
if (setup_data->validate_cn_call_back) {
if (!setup_data->validate_cn_call_back(cn,
crt->raw.p,
crt->raw.len,
c_session,
depth,
*flags == 0,
setup_data->cn_call_back_arg)) {
int ret;

coap_lock_callback_ret(ret, c_session->context,
setup_data->validate_cn_call_back(cn,
crt->raw.p,
crt->raw.len,
c_session,
depth,
*flags == 0,
setup_data->cn_call_back_arg));
if (!ret) {
*flags |= MBEDTLS_X509_BADCERT_CN_MISMATCH;
}
}
Expand Down Expand Up @@ -897,9 +901,9 @@ pki_sni_callback(void *p_info, mbedtls_ssl_context *ssl,
coap_dtls_key_t *new_entry;
pki_sni_entry *pki_sni_entry_list;

new_entry =
m_context->setup_data.validate_sni_call_back(name,
m_context->setup_data.sni_call_back_arg);
coap_lock_callback_ret(new_entry, c_session->context,
m_context->setup_data.validate_sni_call_back(name,
m_context->setup_data.sni_call_back_arg));
if (!new_entry) {
mbedtls_free(name);
return -1;
Expand Down Expand Up @@ -975,10 +979,10 @@ psk_sni_callback(void *p_info, mbedtls_ssl_context *ssl,
const coap_dtls_spsk_info_t *new_entry;
psk_sni_entry *psk_sni_entry_list;

new_entry =
c_session->context->spsk_setup_data.validate_sni_call_back(name,
c_session,
c_session->context->spsk_setup_data.sni_call_back_arg);
coap_lock_callback_ret(new_entry, c_session->context,
c_session->context->spsk_setup_data.validate_sni_call_back(name,
c_session,
c_session->context->spsk_setup_data.sni_call_back_arg));
if (!new_entry) {
mbedtls_free(name);
return -1;
Expand Down
55 changes: 35 additions & 20 deletions src/coap_openssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -762,10 +762,10 @@ coap_dtls_psk_client_callback(SSL *ssl,

lhint.s = temp.s;
lhint.length = temp.length;
cpsk_info =
setup_data->validate_ih_call_back(&lhint,
c_session,
setup_data->ih_call_back_arg);
coap_lock_callback_ret(cpsk_info, c_session->context,
setup_data->validate_ih_call_back(&lhint,
c_session,
setup_data->ih_call_back_arg));

if (cpsk_info == NULL)
return 0;
Expand Down Expand Up @@ -2410,12 +2410,15 @@ tls_verify_call_back(int preverify_ok, X509_STORE_CTX *ctx) {
int length = i2d_X509(x509, NULL);
uint8_t *base_buf;
uint8_t *base_buf2 = base_buf = OPENSSL_malloc(length);
int ret;

/* base_buf2 gets moved to the end */
i2d_X509(x509, &base_buf2);
if (!setup_data->validate_cn_call_back(cn, base_buf, length, session,
depth, preverify_ok,
setup_data->cn_call_back_arg)) {
coap_lock_callback_ret(ret, session->context,
setup_data->validate_cn_call_back(cn, base_buf, length, session,
depth, preverify_ok,
setup_data->cn_call_back_arg));
if (!ret) {
if (depth == 0) {
X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_REJECTED);
} else {
Expand Down Expand Up @@ -2564,8 +2567,11 @@ tls_server_name_call_back(SSL *ssl,
if (i == context->sni_count) {
SSL_CTX *ctx;
coap_dtls_pki_t sni_setup_data;
coap_dtls_key_t *new_entry = setup_data->validate_sni_call_back(sni,
setup_data->sni_call_back_arg);
coap_dtls_key_t *new_entry;

coap_lock_callback_ret(new_entry, session->context,
setup_data->validate_sni_call_back(sni,
setup_data->sni_call_back_arg));
if (!new_entry) {
return SSL_TLSEXT_ERR_ALERT_FATAL;
}
Expand Down Expand Up @@ -2661,10 +2667,12 @@ psk_tls_server_name_call_back(SSL *ssl,
}
if (i == o_context->psk_sni_count) {
SSL_CTX *ctx;
const coap_dtls_spsk_info_t *new_entry =
setup_data->validate_sni_call_back(sni,
c_session,
setup_data->sni_call_back_arg);
const coap_dtls_spsk_info_t *new_entry;

coap_lock_callback_ret(new_entry, c_session->context,
setup_data->validate_sni_call_back(sni,
c_session,
setup_data->sni_call_back_arg));
if (!new_entry) {
return SSL_TLSEXT_ERR_ALERT_FATAL;
}
Expand Down Expand Up @@ -2880,8 +2888,11 @@ tls_client_hello_call_back(SSL *ssl,
/*
* New SNI request
*/
coap_dtls_key_t *new_entry = setup_data->validate_sni_call_back(sni,
setup_data->sni_call_back_arg);
coap_dtls_key_t *new_entry;

coap_lock_callback_ret(new_entry, session->context,
setup_data->validate_sni_call_back(sni,
setup_data->sni_call_back_arg));
if (!new_entry) {
*al = SSL_AD_UNRECOGNIZED_NAME;
return SSL_CLIENT_HELLO_ERROR;
Expand Down Expand Up @@ -3005,10 +3016,13 @@ psk_tls_client_hello_call_back(SSL *ssl,
* New SNI request
*/
psk_sni_entry *tmp_entry;
const coap_dtls_spsk_info_t *new_entry = setup_data->validate_sni_call_back(
sni,
c_session,
setup_data->sni_call_back_arg);
const coap_dtls_spsk_info_t *new_entry;

coap_lock_callback_ret(new_entry, c_session->context,
setup_data->validate_sni_call_back(
sni,
c_session,
setup_data->sni_call_back_arg));
if (!new_entry) {
*al = SSL_AD_UNRECOGNIZED_NAME;
return SSL_CLIENT_HELLO_ERROR;
Expand All @@ -3019,7 +3033,8 @@ psk_tls_client_hello_call_back(SSL *ssl,
(o_context->psk_sni_count+1)*sizeof(sni_entry));
if (tmp_entry) {
o_context->psk_sni_entry_list = tmp_entry;
o_context->psk_sni_entry_list[o_context->psk_sni_count].sni =
o_context->psk_sni_entry_list[o_context->psk_sni_count]
.sni =
OPENSSL_strdup(sni);
if (o_context->psk_sni_entry_list[o_context->psk_sni_count].sni) {
o_context->psk_sni_entry_list[o_context->psk_sni_count].psk_info =
Expand Down
5 changes: 3 additions & 2 deletions src/coap_resource.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,8 +471,9 @@ coap_free_resource(coap_resource_t *resource) {
resource->context->resource_deleted(resource->context, resource->uri_path,
resource->context->observe_user_data);

if (resource->context->release_userdata && resource->user_data)
resource->context->release_userdata(resource->user_data);
if (resource->context->release_userdata && resource->user_data) {
coap_lock_callback(resource->context, resource->context->release_userdata(resource->user_data));
}

/* delete registered attributes */
LL_FOREACH_SAFE(resource->link_attr, attr, tmp) coap_delete_attr(attr);
Expand Down
Loading

0 comments on commit 53e9333

Please sign in to comment.