From 177e400ddd124c62ea3c01c6248e2642833df109 Mon Sep 17 00:00:00 2001 From: Simon Morlat Date: Thu, 15 Oct 2015 16:46:27 +0200 Subject: [PATCH] fix memory leaks and flaws around presence --- coreapi/bellesip_sal/sal_op_events.c | 7 +++- coreapi/bellesip_sal/sal_op_impl.c | 5 +++ coreapi/bellesip_sal/sal_op_presence.c | 52 +++++++++++++------------- coreapi/friend.c | 3 +- coreapi/linphonecore.c | 7 ++-- coreapi/presence.c | 12 +++++- include/sal/sal.h | 1 + tester/presence_tester.c | 33 ++++++++++++++++ 8 files changed, 85 insertions(+), 35 deletions(-) diff --git a/coreapi/bellesip_sal/sal_op_events.c b/coreapi/bellesip_sal/sal_op_events.c index d4a24bd9b..3c328df00 100644 --- a/coreapi/bellesip_sal/sal_op_events.c +++ b/coreapi/bellesip_sal/sal_op_events.c @@ -129,8 +129,7 @@ static void subscribe_process_request_event(void *op_base, const belle_sip_reque if (!op->dialog) { if (strcmp(method,"SUBSCRIBE")==0){ op->dialog=belle_sip_provider_create_dialog(op->base.root->prov,BELLE_SIP_TRANSACTION(server_transaction)); - belle_sip_dialog_set_application_data(op->dialog,op); - sal_op_ref(op); + belle_sip_dialog_set_application_data(op->dialog, sal_op_ref(op)); ms_message("new incoming subscription from [%s] to [%s]",sal_op_get_from(op),sal_op_get_to(op)); }else{ /*this is a NOTIFY*/ handle_notify(op,req,eventname,&body); @@ -227,6 +226,10 @@ int sal_unsubscribe(SalOp *op){ belle_sip_request_t *last_req=belle_sip_transaction_get_request(tr); sal_op_add_body(op,(belle_sip_message_t*)last_req,NULL); belle_sip_refresher_refresh(op->refresher,0); + if (op->dialog){ + belle_sip_dialog_set_application_data(op->dialog, NULL); + sal_op_unref(op); + } return 0; } return -1; diff --git a/coreapi/bellesip_sal/sal_op_impl.c b/coreapi/bellesip_sal/sal_op_impl.c index d19091c96..83dcf032b 100644 --- a/coreapi/bellesip_sal/sal_op_impl.c +++ b/coreapi/bellesip_sal/sal_op_impl.c @@ -814,6 +814,11 @@ void sal_call_set_sdp_handling(SalOp *h, SalOpSDPHandling handling) { void sal_op_cnx_ip_to_0000_if_sendonly_enable(SalOp *op,bool_t yesno) { op->cnx_ip_to_0000_if_sendonly_enabled = yesno; } + bool_t sal_op_cnx_ip_to_0000_if_sendonly_enabled(SalOp *op) { return op->cnx_ip_to_0000_if_sendonly_enabled; } + +bool_t sal_op_is_forked_of(const SalOp *op1, const SalOp *op2){ + return op1->base.call_id && op2->base.call_id && strcmp(op1->base.call_id, op2->base.call_id) == 0; +} diff --git a/coreapi/bellesip_sal/sal_op_presence.c b/coreapi/bellesip_sal/sal_op_presence.c index 6de0a0fbf..94cf35c40 100644 --- a/coreapi/bellesip_sal/sal_op_presence.c +++ b/coreapi/bellesip_sal/sal_op_presence.c @@ -60,29 +60,25 @@ static void presence_process_dialog_terminated(void *ctx, const belle_sip_dialog static void presence_refresher_listener(belle_sip_refresher_t* refresher, void* user_pointer, unsigned int status_code, const char* reason_phrase){ SalOp* op = (SalOp*)user_pointer; - switch(status_code){ - case 481: { - - ms_message("The server or remote ua lost the SUBSCRIBE dialog context. Let's restart a new one."); - belle_sip_refresher_stop(op->refresher); - if (op->dialog) { /*delete previous dialog if any*/ - belle_sip_dialog_set_application_data(op->dialog,NULL); - belle_sip_object_unref(op->dialog); - op->dialog=NULL; - } - - if (sal_op_get_contact_address(op)) { - /*contact is also probably not good*/ - SalAddress* contact=sal_address_clone(sal_op_get_contact_address(op)); - sal_address_set_port(contact,-1); - sal_address_set_domain(contact,NULL); - sal_op_set_contact_address(op,contact); - sal_address_destroy(contact); - } - - sal_subscribe_presence(op,NULL,NULL,-1); - break; + if (status_code >= 300) { + ms_message("The SUBSCRIBE dialog no longer works. Let's restart a new one."); + belle_sip_refresher_stop(op->refresher); + if (op->dialog) { /*delete previous dialog if any*/ + belle_sip_dialog_set_application_data(op->dialog,NULL); + belle_sip_object_unref(op->dialog); + op->dialog=NULL; } + + if (sal_op_get_contact_address(op)) { + /*contact is also probably not good*/ + SalAddress* contact=sal_address_clone(sal_op_get_contact_address(op)); + sal_address_set_port(contact,-1); + sal_address_set_domain(contact,NULL); + sal_op_set_contact_address(op,contact); + sal_address_destroy(contact); + } + /*send a new SUBSCRIBE, that will attempt to establish a new dialog*/ + sal_subscribe_presence(op,NULL,NULL,-1); } } @@ -177,15 +173,19 @@ static SalPresenceModel * process_presence_notification(SalOp *op, belle_sip_req return result; } -static void handle_notify(SalOp *op, belle_sip_request_t *req){ +static void handle_notify(SalOp *op, belle_sip_request_t *req, belle_sip_dialog_t *dialog){ belle_sip_response_t* resp=NULL; belle_sip_server_transaction_t* server_transaction=op->pending_server_trans; belle_sip_header_subscription_state_t* subscription_state_header=belle_sip_message_get_header_by_type(req,belle_sip_header_subscription_state_t); SalSubscribeStatus sub_state; - + if (strcmp("NOTIFY",belle_sip_request_get_method(req))==0) { SalPresenceModel *presence_model = NULL; const char* body = belle_sip_message_get_body(BELLE_SIP_MESSAGE(req)); + + if (op->dialog !=NULL && dialog != op->dialog){ + ms_warning("Receiving a NOTIFY from a dialog we haven't stored (op->dialog=%p dialog=%p)", op->dialog, dialog); + } if (!subscription_state_header || strcasecmp(BELLE_SIP_SUBSCRIPTION_STATE_TERMINATED,belle_sip_header_subscription_state_get_state(subscription_state_header)) ==0) { sub_state=SalSubscribeTerminated; ms_message("Outgoing subscription terminated by remote [%s]",sal_op_get_to(op)); @@ -229,7 +229,7 @@ static void presence_process_request_event(void *op_base, const belle_sip_reques ms_message("new incoming subscription from [%s] to [%s]",sal_op_get_from(op),sal_op_get_to(op)); }else{ /* this is a NOTIFY */ ms_message("Receiving out of dialog notify"); - handle_notify(op,req); + handle_notify(op, req, belle_sip_request_event_get_dialog(event)); return; } } @@ -245,7 +245,7 @@ static void presence_process_request_event(void *op_base, const belle_sip_reques case BELLE_SIP_DIALOG_CONFIRMED: if (strcmp("NOTIFY",method)==0) { - handle_notify(op,req); + handle_notify(op, req, belle_sip_request_event_get_dialog(event)); } else if (strcmp("SUBSCRIBE",method)==0) { /*either a refresh or an unsubscribe*/ if (expires && belle_sip_header_expires_get_expires(expires)>0) { diff --git a/coreapi/friend.c b/coreapi/friend.c index 5dd49bec3..819cdbdcf 100644 --- a/coreapi/friend.c +++ b/coreapi/friend.c @@ -102,7 +102,7 @@ LinphoneFriend *linphone_find_friend_by_out_subscribe(MSList *l, SalOp *op){ LinphoneFriend *lf; for (elem=l;elem!=NULL;elem=elem->next){ lf=(LinphoneFriend*)elem->data; - if (lf->outsub==op) return lf; + if (lf->outsub && (lf->outsub == op || sal_op_is_forked_of(lf->outsub, op))) return lf; } return NULL; } @@ -273,7 +273,6 @@ static void linphone_friend_invalidate_subscription(LinphoneFriend *lf){ void linphone_friend_close_subscriptions(LinphoneFriend *lf){ linphone_friend_unsubscribe(lf); ms_list_for_each(lf->insubs, (MSIterateFunc) sal_notify_presence_close); - } static void _linphone_friend_destroy(LinphoneFriend *lf){ diff --git a/coreapi/linphonecore.c b/coreapi/linphonecore.c index 5ce3b44d2..f02372874 100644 --- a/coreapi/linphonecore.c +++ b/coreapi/linphonecore.c @@ -6319,9 +6319,10 @@ void ui_config_uninit(LinphoneCore* lc) { ms_message("Destroying friends."); if (lc->friends){ - ms_list_for_each(lc->friends,(void (*)(void *))linphone_friend_unref); - ms_list_free(lc->friends); - lc->friends=NULL; + lc->friends = ms_list_free_with_data(lc->friends, (void (*)(void *))linphone_friend_unref); + } + if (lc->subscribers){ + lc->subscribers = ms_list_free_with_data(lc->subscribers, (void (*)(void *))linphone_friend_unref); } if (lc->presence_model) { linphone_presence_model_unref(lc->presence_model); diff --git a/coreapi/presence.c b/coreapi/presence.c index f70f78ae0..f2f16806e 100644 --- a/coreapi/presence.c +++ b/coreapi/presence.c @@ -1448,10 +1448,11 @@ void linphone_core_add_subscriber(LinphoneCore *lc, const char *subscriber, SalO linphone_friend_add_incoming_subscription(fl, op); linphone_friend_set_inc_subscribe_policy(fl,LinphoneSPAccept); fl->inc_subscribe_pending=TRUE; - lc->subscribers=ms_list_append(lc->subscribers,(void *)fl); + lc->subscribers=ms_list_append(lc->subscribers,(void *)linphone_friend_ref(fl)); { char *tmp=linphone_address_as_string(fl->uri); linphone_core_notify_new_subscription_requested(lc,fl,tmp); + linphone_friend_unref(fl); ms_free(tmp); } } @@ -1497,7 +1498,7 @@ void linphone_subscription_new(LinphoneCore *lc, SalOp *op, const char *from){ } else { /* else it is in wait for approval state, because otherwise it is in the friend list.*/ - ms_message("New subscriber found in friend list, in %s state.",__policy_enum_to_str(lf->pol)); + ms_message("New subscriber found in subscriber list, in %s state.",__policy_enum_to_str(lf->pol)); } }else { sal_subscribe_accept(op); @@ -1873,9 +1874,16 @@ void linphone_notify_recv(LinphoneCore *lc, SalOp *op, SalSubscribeStatus ss, Sa lf->subscribe_active=TRUE; linphone_core_notify_notify_presence_received(lc,(LinphoneFriend*)lf); ms_free(tmp); + if (op != lf->outsub){ + /*case of a NOTIFY received out of any dialog*/ + sal_op_release(op); + return; + } }else{ ms_message("But this person is not part of our friend list, so we don't care."); linphone_presence_model_unref(presence); + sal_op_release(op); + return ; } if (ss==SalSubscribeTerminated){ sal_op_release(op); diff --git a/include/sal/sal.h b/include/sal/sal.h index b202224d9..3da8e52c5 100644 --- a/include/sal/sal.h +++ b/include/sal/sal.h @@ -660,6 +660,7 @@ const char *sal_op_get_remote_ua(const SalOp *op); void *sal_op_get_user_pointer(const SalOp *op); const char* sal_op_get_call_id(const SalOp *op); char* sal_op_get_dialog_id(const SalOp *op); +bool_t sal_op_is_forked_of(const SalOp *op1, const SalOp *op2); const SalAddress* sal_op_get_service_route(const SalOp *op); void sal_op_set_service_route(SalOp *op,const SalAddress* service_route); diff --git a/tester/presence_tester.c b/tester/presence_tester.c index c5d5d3ec5..0c5d5e700 100644 --- a/tester/presence_tester.c +++ b/tester/presence_tester.c @@ -346,6 +346,38 @@ static void presence_information(void) { linphone_core_manager_destroy(marie); linphone_core_manager_destroy(pauline); } + + +static void subscribe_presence_forked(){ + LinphoneCoreManager* marie = linphone_core_manager_new("marie_rc"); + LinphoneCoreManager* pauline1 = linphone_core_manager_new(transport_supported(LinphoneTransportTls) ? "pauline_rc" : "pauline_tcp_rc"); + LinphoneCoreManager* pauline2 = linphone_core_manager_new(transport_supported(LinphoneTransportTls) ? "pauline_rc" : "pauline_tcp_rc"); + LinphoneFriend *lf; + MSList *lcs = NULL; + + lcs = ms_list_append(lcs, marie->lc); + lcs = ms_list_append(lcs, pauline1->lc); + lcs = ms_list_append(lcs, pauline2->lc); + + lf = linphone_core_create_friend(marie->lc); + linphone_friend_set_address(lf, pauline1->identity); + linphone_friend_enable_subscribes(lf, TRUE); + + linphone_core_add_friend(marie->lc, lf); + linphone_friend_unref(lf); + + BC_ASSERT_TRUE(wait_for_list(lcs,&pauline1->stat.number_of_NewSubscriptionRequest,1, 10000)); + BC_ASSERT_TRUE(wait_for_list(lcs,&pauline2->stat.number_of_NewSubscriptionRequest,1, 2000)); + /*we should get two notifies*/ + BC_ASSERT_TRUE(wait_for_list(lcs,&marie->stat.number_of_LinphonePresenceActivityOnline,2, 10000)); + + linphone_core_manager_destroy(marie); + linphone_core_manager_destroy(pauline1); + linphone_core_manager_destroy(pauline2); + + ms_list_free(lcs); +} + #define USE_PRESENCE_SERVER 0 #if USE_PRESENCE_SERVER @@ -476,6 +508,7 @@ test_t presence_tests[] = { { "Unsubscribe while subscribing", unsubscribe_while_subscribing }, { "Presence information", presence_information }, { "App managed presence failure", subscribe_failure_handle_by_app }, + { "Presence SUBSCRIBE forked", subscribe_presence_forked }, #if USE_PRESENCE_SERVER { "Subscribe with late publish", test_subscribe_notify_publish }, { "Forked subscribe with late publish", test_forked_subscribe_notify_publish },