[devel] [PATCH 1 of 1] ntfa: fix mutiple mutex lock/unlock on ntfa_cb in saNtfNotificationSend [#1263]

[<pra...@or...>]

 osaf/libs/agents/saf/ntfa/ntfa_api.c |  10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)


In a multithreaded application,saNtfFinalize() may hang forever
leading to deadlock, if one thread tries to call finalize the handle,
when other thread is trying to send a notification.

The reason is saNtfNotificationSend() API takes mutex_lock() on ntfa_cb three
times:
-In the first lock/unlock,it retrieves client record.
-In the second lock/unlock, it verifies notification parameters.
-In the third lock/unlock, it sets notificationId that it has got
  from ntfs.
In saNtfFinalize() API also after receving response from the NTFS, it
tries to free all records for this client and also it releases allocated
memory for the notifications.
If saNtfFinalize() is called for the same client from one thread for which Send() is in
progress in another thread, saNtfFinalize() may get lock on ntfa_cb and it will try to
destroy all the records for this client. But here it will not be able to destroy
the record as Send() API is still using records in the lock free region.
Thus after  taking a lock on ntfa_cb, saNtfFinalize() blocks itself for the client
record to be released by Send(). Since Send() is in progress, in between it will also
try to again take lock on ntfa_cb, but it will not get as it already owned by saNtfFinalize()
which is already blocked because of client resources owned by Send().

As a solution, multiple locks should be avoided in the Send() API. Only while retrieving
client record and notificationHanndle record it should use lock on ntfa_cb.
The reason is saNtfFinalize() will anyway wait for the resoureces taken by Send() to
be released and thus it will not free the memory. In the worst case, saNtfFinalize()
will complete in NTFS_WAIT_TIME, but there will not be any deadlock situation.

diff --git a/osaf/libs/agents/saf/ntfa/ntfa_api.c b/osaf/libs/agents/saf/ntfa/ntfa_api.c
--- a/osaf/libs/agents/saf/ntfa/ntfa_api.c
+++ b/osaf/libs/agents/saf/ntfa/ntfa_api.c
@@ -1398,23 +1398,25 @@ SaAisErrorT saNtfNotificationSend(SaNtfN
 		goto done;
 	}
 
+	osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0);
 	notification_hdl_rec = ncshm_take_hdl(NCS_SERVICE_ID_NTFA, notificationHandle);
 	if (notification_hdl_rec == NULL) {
+		osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0);
 		TRACE("ncshm_take_hdl notificationHandle failed");
 		rc = SA_AIS_ERR_BAD_HANDLE;
 		goto err_free;
 	}
 	notification_hdl_rec->is_longdn_agent_owner = false;
-	osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0);
 	client_handle = notification_hdl_rec->parent_hdl->local_hdl;
-	osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0);
 	/* retrieve client hdl rec */
 	client_rec = ncshm_take_hdl(NCS_SERVICE_ID_NTFA, client_handle);
 	if (client_rec == NULL) {
+		osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0);
 		TRACE("ncshm_take_hdl client_handle failed");
 		rc = SA_AIS_ERR_BAD_HANDLE;
 		goto done_give_hdl;
 	}
+	osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0);
 
     /**
      ** Populate a sync MDS message
@@ -1427,7 +1429,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN
 	send_param->notificationType = notification_hdl_rec->ntfNotificationType;
 	ntfsv_v_data_cp(&send_param->variable_data, &notification_hdl_rec->variable_data);	
 	
-	osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0);
 	/* Check parameters, depending on type */
 	switch (notification_hdl_rec->ntfNotificationType) {
 	case SA_NTF_TYPE_ALARM:
@@ -1486,7 +1487,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN
 		goto done_give_hdls;
 	}
 
-	osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0);
 	/* Send a sync MDS message to obtain a notification id */
 	mds_rc = ntfa_mds_msg_sync_send(&ntfa_cb, &msg, &o_msg, timeout);
 	switch (mds_rc) {
@@ -1506,7 +1506,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN
 		TRACE("ntfa_mds_msg_sync_send FAILED: %u", rc);
 		rc = SA_AIS_ERR_TRY_AGAIN;
 	}
-	osafassert(pthread_mutex_lock(&ntfa_cb.cb_lock) == 0);	
 	if (mds_rc != NCSCC_RC_SUCCESS) {
 		goto done_give_hdls;
 	}
@@ -1556,7 +1555,6 @@ SaAisErrorT saNtfNotificationSend(SaNtfN
  done_give_hdls:
 	 if (o_msg)
 		 ntfa_msg_destroy(o_msg);
-	osafassert(pthread_mutex_unlock(&ntfa_cb.cb_lock) == 0);	 
 	ncshm_give_hdl(client_handle);
  done_give_hdl:
 	ncshm_give_hdl(notificationHandle);