Hi Tai,
Please check my comments [Thien].
Best Regards,
Thien
________________________________
From: Tai Huynh Nguyen <tai.h.ngu...@dektech.com.au>
Sent: Wednesday, February 28, 2024 6:02 PM
To: Dat Tran Quoc Phan <dat.tq.p...@dektech.com.au>; Thang Duc Nguyen
<thang.d.ngu...@dektech.com.au>; Thien Minh Huynh <thien.m.hu...@dektech.com.au>
Cc: opensaf-devel@lists.sourceforge.net <opensaf-devel@lists.sourceforge.net>;
Tai Huynh Nguyen <tai.h.ngu...@dektech.com.au>
Subject: [PATCH 1/1] smf: Fix errors reported by valgrind [#3347]
Fix access uninitialised value, definitely lost and mismatched free()
---
src/smf/common/smfsv_evt.c | 12 ++++++------
src/smf/smfd/SmfCampaignThread.cc | 11 ++++++++++-
src/smf/smfd/SmfExecControl.cc | 3 ++-
src/smf/smfd/SmfUpgradeStep.cc | 2 +-
src/smf/smfd/SmfUtils_ObjExist.cc | 2 +-
src/smf/smfd/smfd_campaign_oi.cc | 10 +++++++++-
6 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/src/smf/common/smfsv_evt.c b/src/smf/common/smfsv_evt.c
index 2df889fca..d28621401 100644
--- a/src/smf/common/smfsv_evt.c
+++ b/src/smf/common/smfsv_evt.c
@@ -168,15 +168,15 @@ uint32_t smf_enc_cbk_rsp(SMF_RESP_EVT *i_evt, NCS_UBAID
*o_ub)
uint32_t rc = NCSCC_RC_SUCCESS;
uint8_t *p8;
- p8 = ncs_enc_reserve_space(o_ub, sizeof(SMF_RESP_EVT));
+ p8 = ncs_enc_reserve_space(o_ub, 8);
[Thien]: Should not decrease this size , It's NBC. Coredump will happens when
upgrade.
should keep and encoding more 4byte
if (p8 == NULL) {
LOG_ER("ncs_enc_reserve_space failed");
goto err;
}
- ncs_encode_64bit(&p8, i_evt->inv_id);
+ ncs_encode_32bit(&p8, i_evt->inv_id);
ncs_encode_32bit(&p8, i_evt->err);
- ncs_enc_claim_space(o_ub, sizeof(SMF_RESP_EVT));
+ ncs_enc_claim_space(o_ub, 8);
return rc;
err:
@@ -229,9 +229,9 @@ uint32_t smf_dec_cbk_rsp(NCS_UBAID *i_ub, SMF_RESP_EVT
*o_evt)
goto err;
}
- p8 = ncs_dec_flatten_space(i_ub, local_data, 8);
- o_evt->inv_id = ncs_decode_64bit(&p8);
- ncs_dec_skip_space(i_ub, 8);
+ p8 = ncs_dec_flatten_space(i_ub, local_data, 4);
+ o_evt->inv_id = ncs_decode_32bit(&p8);
+ ncs_dec_skip_space(i_ub, 4);
p8 = ncs_dec_flatten_space(i_ub, local_data, 4);
o_evt->err = ncs_decode_32bit(&p8);
diff --git a/src/smf/smfd/SmfCampaignThread.cc
b/src/smf/smfd/SmfCampaignThread.cc
index 1f76c3e5a..ba56844ca 100644
--- a/src/smf/smfd/SmfCampaignThread.cc
+++ b/src/smf/smfd/SmfCampaignThread.cc
@@ -493,7 +493,11 @@ void SmfCampaignThread::ntfNotificationCallback(
notification->notificationType);
}
} while (false);
-
+ if (notification->notificationType == SA_NTF_TYPE_STATE_CHANGE) {
+ saNtfNotificationFree(
+ notification->notification.stateChangeNotification
+ .notificationHandle);
+ }
TRACE_LEAVE();
}
/**
@@ -669,6 +673,9 @@ int SmfCampaignThread::sendStateNotification(
return -1;
}
+ memset(ntfStateNot.notificationHeader.correlatedNotifications, 0,
+ ntfStateNot.notificationHeader.numCorrelatedNotifications*
+ sizeof(SaNtfIdentifierT));
/* Notifying object */
size_t length = sizeof(SMF_NOTIFYING_OBJECT);
if (length > static_cast<size_t>(GetSmfMaxDnLength())) {
@@ -682,6 +689,7 @@ int SmfCampaignThread::sendStateNotification(
result = -1;
goto done;
}
+ memset(value, '\0', length + 1);
[Thien]: should use length instead length + 1. Because the last position has
been filled by line "value[length] = '\0';"
memcpy(value, SMF_NOTIFYING_OBJECT, length - 1);
value[length] = '\0';
osaf_extended_name_steal(value,
@@ -700,6 +708,7 @@ int SmfCampaignThread::sendStateNotification(
result = -1;
goto done;
}
+ memset(value, '\0', length + 1);
[Thien]: should use length instead length + 1. Because the last position has
been filled by line "value[length] = '\0';"
memcpy(value, dn.c_str(), length - 1);
value[length] = '\0';
osaf_extended_name_steal(value,
diff --git a/src/smf/smfd/SmfExecControl.cc b/src/smf/smfd/SmfExecControl.cc
index 27584385f..87ebd4a1b 100644
--- a/src/smf/smfd/SmfExecControl.cc
+++ b/src/smf/smfd/SmfExecControl.cc
@@ -89,7 +89,7 @@ bool createBalancedProcs() {
ssproc->setProcName("safSmfProc=SmfBalancedProc" +
std::to_string(procnum));
// Balanced procedures may not be run in parallel
ssproc->setExecLevel(std::to_string(procnum));
- ssproc->setIsMergedProcedure(true); // For cleanup purposes
+ ssproc->setIsMergedProcedure(false); // For cleanup purposes
// Each new procedure holds the balanced group it steps over
ssproc->setBalancedGroup(std::vector<std::string>(itr, iend));
balancedprocs.push_back(ssproc);
@@ -337,6 +337,7 @@ SmfUpgradeStep* mergeStep(SmfUpgradeProcedure* procedure,
TRACE("adding modifications and bundle ref from node:%s",
step->getSwNode().c_str());
newstep->addModifications(step->getModifications());
+ step->getModifications().clear();
procedure->mergeBundleRefRollingToSingleStep(newstep, step);
}
}
diff --git a/src/smf/smfd/SmfUpgradeStep.cc b/src/smf/smfd/SmfUpgradeStep.cc
index e012ef913..6ba5a2c66 100644
--- a/src/smf/smfd/SmfUpgradeStep.cc
+++ b/src/smf/smfd/SmfUpgradeStep.cc
@@ -2180,7 +2180,7 @@ bool SmfUpgradeStep::waitForBundleCmdResult(
}
i_swNodeList.remove(evt->event.swResult.nodeName);
- delete (evt->event.swResult.nodeName);
+ free(evt->event.swResult.nodeName);
delete (evt);
break;
}
diff --git a/src/smf/smfd/SmfUtils_ObjExist.cc
b/src/smf/smfd/SmfUtils_ObjExist.cc
index f89317a08..5278b870c 100644
--- a/src/smf/smfd/SmfUtils_ObjExist.cc
+++ b/src/smf/smfd/SmfUtils_ObjExist.cc
@@ -291,6 +291,6 @@
// Failed. No valid object RDN is found
object_rdn_.clear();
}
-
+ saImmOmClassDescriptionMemoryFree_2(om_handle_, attr_definitions);
return rc;
}
diff --git a/src/smf/smfd/smfd_campaign_oi.cc b/src/smf/smfd/smfd_campaign_oi.cc
index 831b300f8..ffd6063d6 100644
--- a/src/smf/smfd/smfd_campaign_oi.cc
+++ b/src/smf/smfd/smfd_campaign_oi.cc
@@ -1210,22 +1210,30 @@ uint32_t read_config_and_set_control_block(smfd_cb_t
*cb) {
} else {
LOG_NO("smfKeepDuState = %d", *keepDuState);
}
-
+ if (cb->backupCreateCmd != NULL) free(cb->backupCreateCmd);
cb->backupCreateCmd = strdup(backupCreateCmd);
+ if (cb->bundleCheckCmd != NULL) free(cb->bundleCheckCmd);
cb->bundleCheckCmd = strdup(bundleCheckCmd);
+ if (cb->nodeCheckCmd != NULL) free(cb->nodeCheckCmd);
cb->nodeCheckCmd = strdup(nodeCheckCmd);
+ if (cb->repositoryCheckCmd != NULL) free(cb->repositoryCheckCmd);
cb->repositoryCheckCmd = strdup(repositoryCheckCmd);
+ if (cb->clusterRebootCmd != NULL) free(cb->clusterRebootCmd);
cb->clusterRebootCmd = strdup(clusterRebootCmd);
cb->adminOpTimeout = *adminOpTimeout;
cb->cliTimeout = *cliTimeout;
cb->rebootTimeout = *rebootTimeout;
if ((smfNodeBundleActCmd != NULL) && (strcmp(smfNodeBundleActCmd, "") != 0))
{
+ if (cb->nodeBundleActCmd != NULL) free(cb->nodeBundleActCmd);
cb->nodeBundleActCmd = strdup(smfNodeBundleActCmd);
}
+ if (cb->smfSiSwapSiName != NULL) free (cb->smfSiSwapSiName);
cb->smfSiSwapSiName = strdup(smfSiSwapSiName);
cb->smfSiSwapMaxRetry = *smfSiSwapMaxRetry;
cb->smfCampMaxRestart = *smfCampMaxRestart;
+ if (cb->smfImmPersistCmd != NULL) free(cb->smfImmPersistCmd);
cb->smfImmPersistCmd = strdup(smfImmPersistCmd);
+ if (cb->smfNodeRebootCmd != NULL) free(cb->smfNodeRebootCmd);
cb->smfNodeRebootCmd = strdup(smfNodeRebootCmd);
cb->smfInactivatePbeDuringUpgrade = *smfInactivatePbeDuringUpgrade;
cb->smfVerifyEnable = *smfVerifyEnable;
--
2.25.1
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
