Re: [devel] [PATCH 1/1] amfa: Fix api internal check to avoid fatal mutex unlock [#2548]
Hi Hans,
Thanks for reviewing the patch. I've made the necessary changes based on
your comments before having the patch pushed.
Regards,
Nguyen
On 3/9/2018 4:58 PM, Hans Nordebäck wrote:
ack, review only. Some comments below marked with [HansN].
/Thanks HansN
On 03/06/2018 07:39 AM, Nguyen Luu wrote:
Current check for the required setting of the SA_AMF_COMPONENT_NAME
env variable in some amf api's (ComponentRegister, QuiescingComplete)
would crash the invoking process if that env variable was missed
to be set for some reason, as the agent lib tries, during cleanup,
to unlock a mutex which it has not previously locked yet.
---
src/amf/agent/amf_agent.cc | 51
--
1 file changed, 31 insertions(+), 20 deletions(-)
diff --git a/src/amf/agent/amf_agent.cc b/src/amf/agent/amf_agent.cc
index f0ed7a8..a162af1 100644
--- a/src/amf/agent/amf_agent.cc
+++ b/src/amf/agent/amf_agent.cc
@@ -2,6 +2,7 @@
*
* (C) Copyright 2008 The OpenSAF Foundation
* Copyright (C) 2017, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (C) 2018 Ericsson AB. All Rights Reserved.
*
* This program is distributed in the hope that it will be useful, but
* WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY
@@ -13,6 +14,7 @@
* licensing terms.
*
* Author(s): Emerson Network Power
+ * Ericsson
*
*/
@@ -272,7 +274,7 @@ SaAisErrorT AmfAgent::Dispatch(SaAmfHandleT
hdl, SaDispatchFlagsT flags) {
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
- /* acquire cb read lock */
+ /* acquire cb write lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
hdl))) {
@@ -293,7 +295,7 @@ SaAisErrorT AmfAgent::Dispatch(SaAmfHandleT hdl,
SaDispatchFlagsT flags) {
pend_fin = cb->pend_fin;
done:
- /* release cb read lock and return handles */
+ /* release cb write lock and return handles */
if (cb) {
m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
ncshm_give_hdl(gl_ava_hdl);
@@ -356,7 +358,7 @@ SaAisErrorT AmfAgent::Finalize(SaAmfHandleT hdl) {
uninstall_osafAmfSCStatusChangeCallback();
}
- /* acquire cb read lock */
+ /* acquire cb write lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
/* retrieve hdl rec */
@@ -394,7 +396,7 @@ SaAisErrorT AmfAgent::Finalize(SaAmfHandleT hdl) {
cb->pend_fin++;
done:
- /* release cb read lock and return handles */
+ /* release cb read write and return handles */
if (cb) {
m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
ncshm_give_hdl(gl_ava_hdl);
@@ -457,8 +459,7 @@ SaAisErrorT
AmfAgent::ComponentRegister(SaAmfHandleT hdl,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl))) {
[HansN] the agent is c++, so use if (!(cb = static_cast*>(ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
@@ -627,8 +628,7 @@ SaAisErrorT
AmfAgent::ComponentUnregister(SaAmfHandleT hdl,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl))) {
[HansN] if (!(cb = static_cast*>(ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
@@ -636,6 +636,13 @@ SaAisErrorT
AmfAgent::ComponentUnregister(SaAmfHandleT hdl,
/* acquire cb read lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
+
+ if (!m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ TRACE_2("The SA_AMF_COMPONENT_NAME environment variable is NULL");
+ rc = SA_AIS_ERR_LIBRARY;
+ goto done;
+ }
+
/* Version is previously set in in initialize function */
if (ava_B4_ver_used(cb)) {
TRACE_2("Invalid AMF version, B 4.1");
@@ -1373,14 +1380,20 @@ SaAisErrorT
AmfAgent::CSIQuiescingComplete(SaAmfHandleT hdl, SaInvocationT inv,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA,
gl_ava_hdl))) {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
/* acquire cb read lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
+
+ if (!m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ TRACE_2("The SA_AMF_COMPONENT_NAME environment variable is NULL");
+ rc = SA_AIS_ERR_LIBRARY;
+ goto done;
+ }
+
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NC
Re: [devel] [PATCH 1/1] amfa: Fix api internal check to avoid fatal mutex unlock [#2548]
ack, review only. Some comments below marked with [HansN].
/Thanks HansN
On 03/06/2018 07:39 AM, Nguyen Luu wrote:
Current check for the required setting of the SA_AMF_COMPONENT_NAME
env variable in some amf api's (ComponentRegister, QuiescingComplete)
would crash the invoking process if that env variable was missed
to be set for some reason, as the agent lib tries, during cleanup,
to unlock a mutex which it has not previously locked yet.
---
src/amf/agent/amf_agent.cc | 51 --
1 file changed, 31 insertions(+), 20 deletions(-)
diff --git a/src/amf/agent/amf_agent.cc b/src/amf/agent/amf_agent.cc
index f0ed7a8..a162af1 100644
--- a/src/amf/agent/amf_agent.cc
+++ b/src/amf/agent/amf_agent.cc
@@ -2,6 +2,7 @@
*
* (C) Copyright 2008 The OpenSAF Foundation
* Copyright (C) 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (C) 2018 Ericsson AB. All Rights Reserved.
*
* This program is distributed in the hope that it will be useful, but
* WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
@@ -13,6 +14,7 @@
* licensing terms.
*
* Author(s): Emerson Network Power
+ *Ericsson
*
*/
@@ -272,7 +274,7 @@ SaAisErrorT AmfAgent::Dispatch(SaAmfHandleT hdl, SaDispatchFlagsT flags) {
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
- /* acquire cb read lock */
+ /* acquire cb write lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, hdl))) {
@@ -293,7 +295,7 @@ SaAisErrorT AmfAgent::Dispatch(SaAmfHandleT hdl,
SaDispatchFlagsT flags) {
pend_fin = cb->pend_fin;
done:
- /* release cb read lock and return handles */
+ /* release cb write lock and return handles */
if (cb) {
m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
ncshm_give_hdl(gl_ava_hdl);
@@ -356,7 +358,7 @@ SaAisErrorT AmfAgent::Finalize(SaAmfHandleT hdl) {
uninstall_osafAmfSCStatusChangeCallback();
}
- /* acquire cb read lock */
+ /* acquire cb write lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_WRITE);
/* retrieve hdl rec */
@@ -394,7 +396,7 @@ SaAisErrorT AmfAgent::Finalize(SaAmfHandleT hdl) {
cb->pend_fin++;
done:
- /* release cb read lock and return handles */
+ /* release cb read write and return handles */
if (cb) {
m_NCS_UNLOCK(&cb->lock, NCS_LOCK_WRITE);
ncshm_give_hdl(gl_ava_hdl);
@@ -457,8 +459,7 @@ SaAisErrorT AmfAgent::ComponentRegister(SaAmfHandleT hdl,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl))) {
[HansN] the agent is c++, so use if (!(cb = static_cast*>(ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
@@ -627,8 +628,7 @@ SaAisErrorT AmfAgent::ComponentUnregister(SaAmfHandleT hdl,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl))) {
[HansN] if (!(cb = static_cast*>(ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
@@ -636,6 +636,13 @@ SaAisErrorT AmfAgent::ComponentUnregister(SaAmfHandleT hdl,
/* acquire cb read lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
+
+ if (!m_AVA_FLAG_IS_COMP_NAME(cb)) {
+TRACE_2("The SA_AMF_COMPONENT_NAME environment variable is NULL");
+rc = SA_AIS_ERR_LIBRARY;
+goto done;
+ }
+
/* Version is previously set in in initialize function */
if (ava_B4_ver_used(cb)) {
TRACE_2("Invalid AMF version, B 4.1");
@@ -1373,14 +1380,20 @@ SaAisErrorT AmfAgent::CSIQuiescingComplete(SaAmfHandleT
hdl, SaInvocationT inv,
goto done;
}
/* retrieve AvA CB */
- if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl)) ||
- !m_AVA_FLAG_IS_COMP_NAME(cb)) {
+ if (!(cb = (AVA_CB *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, gl_ava_hdl))) {
TRACE_4("SA_AIS_ERR_LIBRARY: Unable to retrieve cb handle");
rc = SA_AIS_ERR_LIBRARY;
goto done;
}
/* acquire cb read lock */
m_NCS_LOCK(&cb->lock, NCS_LOCK_READ);
+
+ if (!m_AVA_FLAG_IS_COMP_NAME(cb)) {
+TRACE_2("The SA_AMF_COMPONENT_NAME environment variable is NULL");
+rc = SA_AIS_ERR_LIBRARY;
+goto done;
+ }
+
/* retrieve hdl rec */
if (!(hdl_rec = (AVA_HDL_REC *)ncshm_take_hdl(NCS_SERVICE_ID_AVA, hdl))) {
rc = SA_AIS_ERR_BAD_HANDLE;
@@ -1591,7 +1604,7 @@ SaAisErrorT AmfAgent::ProtectionGroupTrack(
goto done;
}
/* acquire cb read lock */
- m_NCS_LOCK(&cb->lock, NC
