From c410956a3132598d174de75592eb14e4298e5a29 Mon Sep 17 00:00:00 2001 From: Andy Green Date: Sat, 4 Jul 2020 21:16:49 +0100 Subject: [PATCH] ss: event_helper handles destroy requests itself Callbacks can ask the caller to, eg, destroy the ss handle now. But some callback returns are handled and produced inside other helper apis, eg lws_ss_backoff() may have to had fulfilled the callback request to destroy the ss... therefore it has to signal to its caller, and its callers have to check and exit their flow accordingly. --- include/libwebsockets/lws-secure-streams.h | 2 + lib/secure-streams/protocols/ss-h1.c | 30 ++++++++------- lib/secure-streams/protocols/ss-mqtt.c | 5 ++- lib/secure-streams/protocols/ss-raw.c | 9 +++-- lib/secure-streams/protocols/ss-ws.c | 17 ++++----- lib/secure-streams/secure-streams.c | 44 ++++++++++++++-------- 6 files changed, 66 insertions(+), 41 deletions(-) diff --git a/include/libwebsockets/lws-secure-streams.h b/include/libwebsockets/lws-secure-streams.h index b85e437a4..c3d20da9f 100644 --- a/include/libwebsockets/lws-secure-streams.h +++ b/include/libwebsockets/lws-secure-streams.h @@ -257,6 +257,8 @@ enum lws_ss_state_return_t { LWSSSSRET_OK = 0, LWSSSSRET_DISCONNECT_ME = -1, LWSSSSRET_DESTROY_ME = -2, + + LWSSSSRET_SS_HANDLE_DESTROYED = -3, }; /** diff --git a/lib/secure-streams/protocols/ss-h1.c b/lib/secure-streams/protocols/ss-h1.c index cf07fd8d6..0e267c88f 100644 --- a/lib/secure-streams/protocols/ss-h1.c +++ b/lib/secure-streams/protocols/ss-h1.c @@ -178,13 +178,15 @@ secstream_h1(struct lws *wsi, enum lws_callback_reasons reason, void *user, lwsl_info("%s: h: %p, %s CLIENT_CONNECTION_ERROR: %s\n", __func__, h, h->policy->streamtype, in ? (char *)in : "(null)"); /* already disconnected, no action for DISCONNECT_ME */ - if (lws_ss_event_helper(h, LWSSSCS_UNREACHABLE) == - LWSSSSRET_DESTROY_ME) { - lws_ss_destroy(&h); + if (lws_ss_event_helper(h, LWSSSCS_UNREACHABLE)) + /* h has been destroyed */ break; - } + h->wsi = NULL; - lws_ss_backoff(h); + lwsl_notice("a4\n"); + if (lws_ss_backoff(h)) + /* was destroyed */ + return -1; break; case LWS_CALLBACK_CLIENT_HTTP_REDIRECT: @@ -206,14 +208,14 @@ secstream_h1(struct lws *wsi, enum lws_callback_reasons reason, void *user, //bad = status != 200; //lws_cancel_service(lws_get_context(wsi)); /* abort poll wait */ if (h->policy && !(h->policy->flags & LWSSSPOLF_OPPORTUNISTIC) && - !h->txn_ok && !wsi->context->being_destroyed) - lws_ss_backoff(h); - else + !h->txn_ok && !wsi->context->being_destroyed) { + if (lws_ss_backoff(h)) + break; + } else h->seqstate = SSSEQ_IDLE; /* already disconnected, no action for DISCONNECT_ME */ - if (lws_ss_event_helper(h, LWSSSCS_DISCONNECTED) == - LWSSSSRET_DESTROY_ME) - lws_ss_destroy(&h); + lws_ss_event_helper(h, LWSSSCS_DISCONNECTED); + /* may have been destroyed */ break; @@ -243,9 +245,9 @@ secstream_h1(struct lws *wsi, enum lws_callback_reasons reason, void *user, h->retry = 0; h->seqstate = SSSEQ_CONNECTED; lws_sul_cancel(&h->sul); - if (lws_ss_event_helper(h, LWSSSCS_CONNECTED)) { + if (lws_ss_event_helper(h, LWSSSCS_CONNECTED)) + /* was destroyed */ return -1; - } /* * Since it's an http transaction we initiated... this is @@ -459,9 +461,11 @@ malformed: if (h->u.http.good_respcode) { if (lws_ss_event_helper(h, LWSSSCS_QOS_ACK_REMOTE)) + /* was destroyed */ return -1; } else if (lws_ss_event_helper(h, LWSSSCS_QOS_NACK_REMOTE)) + /* was destroyed */ return -1; //bad = status != 200; diff --git a/lib/secure-streams/protocols/ss-mqtt.c b/lib/secure-streams/protocols/ss-mqtt.c index 80aa85881..0e857258f 100644 --- a/lib/secure-streams/protocols/ss-mqtt.c +++ b/lib/secure-streams/protocols/ss-mqtt.c @@ -50,6 +50,7 @@ secstream_mqtt(struct lws *wsi, enum lws_callback_reasons reason, void *user, } lws_ss_backoff(h); + /* may have been destroyed */ break; case LWS_CALLBACK_MQTT_CLIENT_CLOSED: @@ -73,7 +74,9 @@ secstream_mqtt(struct lws *wsi, enum lws_callback_reasons reason, void *user, if (h->policy && !(h->policy->flags & LWSSSPOLF_OPPORTUNISTIC) && !h->txn_ok && !wsi->context->being_destroyed) - lws_ss_backoff(h); + if (lws_ss_backoff(h)) + /* has been destroyed */ + return -1; break; case LWS_CALLBACK_MQTT_CLIENT_ESTABLISHED: diff --git a/lib/secure-streams/protocols/ss-raw.c b/lib/secure-streams/protocols/ss-raw.c index 7ed740bb6..1d0c8a255 100644 --- a/lib/secure-streams/protocols/ss-raw.c +++ b/lib/secure-streams/protocols/ss-raw.c @@ -46,6 +46,7 @@ secstream_raw(struct lws *wsi, enum lws_callback_reasons reason, void *user, lws_ss_event_helper(h, LWSSSCS_UNREACHABLE); h->wsi = NULL; lws_ss_backoff(h); + /* may have been destroyed */ break; case LWS_CALLBACK_RAW_CLOSE: @@ -58,9 +59,11 @@ secstream_raw(struct lws *wsi, enum lws_callback_reasons reason, void *user, h->wsi = NULL; if (h->policy && !(h->policy->flags & LWSSSPOLF_OPPORTUNISTIC) && !h->txn_ok && !wsi->context->being_destroyed) - lws_ss_backoff(h); - if (lws_ss_event_helper(h, LWSSSCS_DISCONNECTED)) - lws_ss_destroy(&h); + if (lws_ss_backoff(h)) + /* has been destroyed */ + break; + /* wsi is going down anyway */ + lws_ss_event_helper(h, LWSSSCS_DISCONNECTED); break; case LWS_CALLBACK_RAW_CONNECTED: diff --git a/lib/secure-streams/protocols/ss-ws.c b/lib/secure-streams/protocols/ss-ws.c index 06786e9b8..10a3bd5fe 100644 --- a/lib/secure-streams/protocols/ss-ws.c +++ b/lib/secure-streams/protocols/ss-ws.c @@ -41,31 +41,30 @@ secstream_ws(struct lws *wsi, enum lws_callback_reasons reason, void *user, in ? (char *)in : "(null)"); if (!h) break; - if (lws_ss_event_helper(h, LWSSSCS_UNREACHABLE)) { - lws_ss_destroy(&h); + if (lws_ss_event_helper(h, LWSSSCS_UNREACHABLE)) + /* h has been destroyed */ break; - } + h->wsi = NULL; lws_ss_backoff(h); + /* may have been destroyed */ break; case LWS_CALLBACK_CLIENT_CLOSED: if (!h) break; lws_sul_cancel(&h->sul_timeout); - f = lws_ss_event_helper(h, LWSSSCS_DISCONNECTED); + if (lws_ss_event_helper(h, LWSSSCS_DISCONNECTED)) + /* has been destroyed */ + break; if (h->wsi) lws_set_opaque_user_data(h->wsi, NULL); h->wsi = NULL; - if (f) { - lws_ss_destroy(&h); - break; - } - if (h->policy && !(h->policy->flags & LWSSSPOLF_OPPORTUNISTIC) && !h->txn_ok && !wsi->context->being_destroyed) lws_ss_backoff(h); + /* may have been destroyed */ break; case LWS_CALLBACK_CLIENT_ESTABLISHED: diff --git a/lib/secure-streams/secure-streams.c b/lib/secure-streams/secure-streams.c index 654facf1c..8b9d640ea 100644 --- a/lib/secure-streams/secure-streams.c +++ b/lib/secure-streams/secure-streams.c @@ -99,6 +99,7 @@ lws_ss_event_helper(lws_ss_handle_t *h, lws_ss_constate_t cs) lws_set_timeout(h->wsi, 1, LWS_TO_KILL_ASYNC); h->wsi = NULL; /* stop destroy trying to repeat this */ if (n == LWSSSSRET_DESTROY_ME) { + lwsl_info("%s: DESTROYING ss %p\n", __func__, h); lws_ss_destroy(&h); return 1; } @@ -114,7 +115,7 @@ lws_ss_event_helper(lws_ss_handle_t *h, lws_ss_constate_t cs) if (h->wsi) lws_set_timeout(h->wsi, 1, LWS_TO_KILL_ASYNC); if (n == LWSSSSRET_DESTROY_ME) { - lwsl_info("%s: ss %p asks to be destroyed\n", __func__, h); + lwsl_info("%s: DESTROYING ss %p\n", __func__, h); /* disconnect ss from the wsi */ if (h->wsi) lws_set_opaque_user_data(h->wsi, NULL); @@ -209,7 +210,9 @@ lws_ss_backoff(lws_ss_handle_t *h) if (!conceal) { lwsl_info("%s: ss %p: abandon conn attempt \n",__func__, h); h->seqstate = SSSEQ_IDLE; - lws_ss_event_helper(h, LWSSSCS_ALL_RETRIES_FAILED); + if (lws_ss_event_helper(h, LWSSSCS_ALL_RETRIES_FAILED)) + lwsl_notice("%s: was desroyed on ARF event\n", __func__); + /* may have been destroyed */ return 1; } @@ -461,11 +464,16 @@ _lws_ss_client_connect(lws_ss_handle_t *h, int is_retry) h->txn_ok = 0; if (lws_ss_event_helper(h, LWSSSCS_CONNECTING)) - return -1; + return LWSSSSRET_SS_HANDLE_DESTROYED; if (!lws_client_connect_via_info(&i)) { - lws_ss_event_helper(h, LWSSSCS_UNREACHABLE); - lws_ss_backoff(h); + if (lws_ss_event_helper(h, LWSSSCS_UNREACHABLE)) { + /* was destroyed */ + lwsl_err("%s: client connect UNREACHABLE destroyed the ss\n", __func__); + return LWSSSSRET_SS_HANDLE_DESTROYED; + } + if (lws_ss_backoff(h)) + return LWSSSSRET_SS_HANDLE_DESTROYED; return 1; } @@ -667,8 +675,11 @@ late_bail: ) #endif )) - if (_lws_ss_client_connect(h, 0)) - lws_ss_backoff(h); + if (_lws_ss_client_connect(h, 0)) { + if (lws_ss_backoff(h)) + /* has been destroyed */ + return 1; + } return 0; } @@ -714,7 +725,7 @@ lws_ss_destroy(lws_ss_handle_t **ppss) lws_dll2_remove(&h->list); lws_dll2_remove(&h->to_list); lws_sul_cancel(&h->sul_timeout); - /* no need to worry about return code since we are anyway destroying */ + lws_ss_event_helper(h, LWSSSCS_DESTROYING); lws_pt_unlock(pt); @@ -736,6 +747,8 @@ lws_ss_destroy(lws_ss_handle_t **ppss) void lws_ss_request_tx(lws_ss_handle_t *h) { + int n; + lwsl_info("%s: wsi %p\n", __func__, h->wsi); if (h->wsi) { @@ -771,7 +784,11 @@ lws_ss_request_tx(lws_ss_handle_t *h) * Retries operate via lws_ss_request_tx(), explicitly ask for a * reconnection to clear the retry limit */ - if (_lws_ss_client_connect(h, 1)) + n = _lws_ss_client_connect(h, 1); + if (n == LWSSSSRET_SS_HANDLE_DESTROYED) + return; + + if (n) lws_ss_backoff(h); } @@ -859,12 +876,9 @@ lws_ss_to_cb(lws_sorted_usec_list_t *sul) lwsl_info("%s: ss %p timeout fired\n", __func__, h); - if (lws_ss_event_helper(h, LWSSSCS_TIMEOUT) == LWSSSSRET_DESTROY_ME) - /* - * We're called from the sul handler, caller doesn't have any - * copy of the ss handle - */ - lws_ss_destroy(&h); + lws_ss_event_helper(h, LWSSSCS_TIMEOUT); + + /* may have been destroyed */ } void