Hi Surbhi,
I have tested with imm test. The patch working well.
ACK from me.
Best Regards,
Thien
-----Original Message-----
From: ztrisur <surbhi.tripa...@dektech.com.au>
Sent: Monday, March 22, 2021 6:30 AM
To: Thang Duc Nguyen <thang.d.ngu...@dektech.com.au>; Minh Hon Chau
<minh.c...@dektech.com.au>
Cc: opensaf-devel@lists.sourceforge.net
Subject: [devel] [PATCH 1/1] imm: valgrind reports invalid read in imm agent
[#2656]
The attrName.size includes the null character in the attrName.buf.
So only 'size' memory should be allocated and copied and not (size+1)
strncpy will copy minimum of attrName.size and attrName.buf, In normal
scenarios this should be attrName.buf
But in case buf is corrupt and extends beyond attrName.size. attrName.size
characters are copied, in this case setting the last character to '\0'
is important
---
src/imm/agent/imma_oi_api.cc | 2 ++
src/imm/agent/imma_om_api.cc | 27 +++++++++++++++++----------
src/imm/agent/imma_proc.cc | 6 +++---
src/imm/immnd/ImmModel.cc | 2 +-
4 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/imm/agent/imma_oi_api.cc b/src/imm/agent/imma_oi_api.cc index
0992c8225..8e9a1f2b0 100644
--- a/src/imm/agent/imma_oi_api.cc
+++ b/src/imm/agent/imma_oi_api.cc
@@ -2696,6 +2696,7 @@ static SaAisErrorT rt_object_update_common(
p->attrValue.attrName.buf = (char *)malloc(p->attrValue.attrName.size);
strncpy(p->attrValue.attrName.buf, attrMod->modAttr.attrName,
p->attrValue.attrName.size);
+ p->attrValue.attrName.buf[p->attrValue.attrName.size-1] = 0;
p->attrValue.attrValuesNumber = attrMod->modAttr.attrValuesNumber;
p->attrValue.attrValueType = attrMod->modAttr.attrValueType; @@ -3095,6
+3096,7 @@ static SaAisErrorT rt_object_create_common(
/*alloc-4 */
p->n.attrName.buf = (char *)malloc(p->n.attrName.size);
strncpy(p->n.attrName.buf, attr->attrName, p->n.attrName.size);
+ p->n.attrName.buf[p->n.attrName.size-1] = 0;
p->n.attrValuesNumber = attr->attrValuesNumber;
p->n.attrValueType = attr->attrValueType; diff --git
a/src/imm/agent/imma_om_api.cc b/src/imm/agent/imma_om_api.cc index
f024cc997..b5922755e 100644
--- a/src/imm/agent/imma_om_api.cc
+++ b/src/imm/agent/imma_om_api.cc
@@ -2056,8 +2056,8 @@ static SaAisErrorT ccb_object_create_common(
/*alloc-4 */
p->n.attrName.buf = (char *)malloc(p->n.attrName.size);
-
strncpy(p->n.attrName.buf, attr->attrName, p->n.attrName.size);
+ p->n.attrName.buf[p->n.attrName.size-1] = 0;
p->n.attrValuesNumber = attr->attrValuesNumber;
p->n.attrValueType = attr->attrValueType; @@ -2589,6 +2589,7 @@ static
SaAisErrorT ccb_object_modify_common(
p->attrValue.attrName.buf = (char *)malloc(p->attrValue.attrName.size);
strncpy(p->attrValue.attrName.buf, attrMod->modAttr.attrName,
p->attrValue.attrName.size);
+ p->attrValue.attrName.buf[p->attrValue.attrName.size-1] = 0;
p->attrValue.attrValuesNumber = attrMod->modAttr.attrValuesNumber;
p->attrValue.attrValueType = attrMod->modAttr.attrValueType; @@ -4050,6
+4051,7 @@ static SaAisErrorT admin_op_invoke_common(
/*alloc-3 */
p->paramName.buf = (char *)malloc(p->paramName.size);
strncpy(p->paramName.buf, param->paramName, p->paramName.size);
+ p->paramName.buf[p->paramName.size-1] = 0;
p->paramType = param->paramType;
/*alloc-4 */
@@ -5048,6 +5050,7 @@ SaAisErrorT saImmOmClassCreate_2(
}
p->d.attrName.buf = (char *)malloc(p->d.attrName.size); /* alloc-3 */
strncpy(p->d.attrName.buf, attr->attrName, p->d.attrName.size);
+ p->d.attrName.buf[p->d.attrName.size-1] = 0;
p->d.attrValueType = attr->attrValueType;
if (!imma_proc_is_valid_type((SaImmValueTypeT)p->d.attrValueType)) { @@
-5087,6 +5090,7 @@ SaAisErrorT saImmOmClassCreate_2(
sysattr->d.attrName.buf =
(char *)malloc(sysattr->d.attrName.size); /*alloc-3 */
strncpy(sysattr->d.attrName.buf, sysaClName, sysattr->d.attrName.size);
+ sysattr->d.attrName.buf[sysattr->d.attrName.size-1] = 0;
sysattr->d.attrValueType = SA_IMM_ATTR_SASTRINGT;
if (classCategory == SA_IMM_CLASS_CONFIG) {
sysattr->d.attrFlags |= SA_IMM_ATTR_CONFIG; @@ -5116,6 +5120,7 @@
SaAisErrorT saImmOmClassCreate_2(
sysattr->d.attrName.buf =
(char *)malloc(sysattr->d.attrName.size); /*alloc-3 */
strncpy(sysattr->d.attrName.buf, sysaAdmName, sysattr->d.attrName.size);
+ sysattr->d.attrName.buf[sysattr->d.attrName.size-1] = 0;
sysattr->d.attrValueType = SA_IMM_ATTR_SASTRINGT;
/* Should this attribute really be a config attribute ?
Should it really be allowed to be persistent ? */ @@ -5142,6 +5147,7 @@
SaAisErrorT saImmOmClassCreate_2(
sysattr->d.attrName.buf =
(char *)malloc(sysattr->d.attrName.size); /*alloc-3 */
strncpy(sysattr->d.attrName.buf, sysaImplName, sysattr->d.attrName.size);
+ sysattr->d.attrName.buf[sysattr->d.attrName.size-1] = 0;
sysattr->d.attrValueType = SA_IMM_ATTR_SASTRINGT;
/* Should this attribute really be a config attribute ?
Should it really be allowed to be persistent ?
@@ -5380,10 +5386,10 @@ SaAisErrorT saImmOmClassDescriptionGet_2(
IMMSV_ATTR_DEFINITION *q = &(p->d);
attr[i] = (SaImmAttrDefinitionT_2 *)malloc(
sizeof(SaImmAttrDefinitionT_2)); /*alloc-2 */
- attr[i]->attrName = (char *)malloc(q->attrName.size + 1); /*alloc-3 */
+ attr[i]->attrName = (char *)malloc(q->attrName.size); /*alloc-3
+ */
strncpy(attr[i]->attrName, (const char *)q->attrName.buf,
- q->attrName.size + 1);
- attr[i]->attrName[q->attrName.size] = 0;
+ q->attrName.size);
+ attr[i]->attrName[q->attrName.size-1] = 0;
attr[i]->attrValueType = (SaImmValueTypeT)q->attrValueType;
attr[i]->attrFlags = q->attrFlags;
/* attr[i]->attrNtfId = q->attrNtfId; */ @@ -6337,10 +6343,10 @@
static SaAisErrorT accessor_get_common(SaImmAccessorHandleT accessorHandle,
IMMSV_ATTR_VALUES *q = &(p->n);
attr[i] = (SaImmAttrValuesT_2 *)calloc(
1, sizeof(SaImmAttrValuesT_2)); /* alloc-2 */
- attr[i]->attrName = (char *)malloc(q->attrName.size + 1); /* alloc-3 */
+ attr[i]->attrName = (char *)malloc(q->attrName.size); /*
+ alloc-3 */
strncpy(attr[i]->attrName, (const char *)q->attrName.buf,
- q->attrName.size + 1);
- attr[i]->attrName[q->attrName.size] = 0; /*redundant. */
+ q->attrName.size);
+ attr[i]->attrName[q->attrName.size-1] = 0; /*redundant. */
attr[i]->attrValuesNumber = q->attrValuesNumber;
attr[i]->attrValueType = (SaImmValueTypeT)q->attrValueType;
@@ -7042,6 +7048,7 @@ SaAisErrorT immsv_sync(SaImmHandleT immHandle, const
SaImmClassNameT className,
/*alloc-4 */
p->n.attrName.buf = (char *)malloc(p->n.attrName.size);
strncpy(p->n.attrName.buf, attr->attrName, p->n.attrName.size);
+ p->n.attrName.buf[p->n.attrName.size-1] = 0;
p->n.attrValuesNumber = attr->attrValuesNumber;
p->n.attrValueType = attr->attrValueType; @@ -8113,10 +8120,10 @@
searchresult:
IMMSV_ATTR_VALUES *q = &(p->n);
attr[i] = (SaImmAttrValuesT_2 *)calloc(
1, sizeof(SaImmAttrValuesT_2)); /*alloc-2 */
- attr[i]->attrName = (char *)malloc(q->attrName.size + 1); /*alloc-3 */
+ attr[i]->attrName = (char *)malloc(q->attrName.size); /*alloc-3
+ */
strncpy(attr[i]->attrName, (const char *)q->attrName.buf,
- q->attrName.size + 1);
- attr[i]->attrName[q->attrName.size] = 0; /*redundant. */
+ q->attrName.size);
+ attr[i]->attrName[q->attrName.size-1] = 0; /*redundant. */
attr[i]->attrValuesNumber = q->attrValuesNumber;
attr[i]->attrValueType = (SaImmValueTypeT)q->attrValueType;
diff --git a/src/imm/agent/imma_proc.cc b/src/imm/agent/imma_proc.cc index
dafd42391..ea035827f 100644
--- a/src/imm/agent/imma_proc.cc
+++ b/src/imm/agent/imma_proc.cc
@@ -2056,10 +2056,10 @@ static void imma_proc_ccbaug_setup(IMMA_CLIENT_NODE
*cl_node,
IMMSV_ATTR_VALUES *q = &(p->n);
attr[i] = (SaImmAttrValuesT_2 *)calloc(
1, sizeof(SaImmAttrValuesT_2)); /*alloc-2 */
- attr[i]->attrName = (char *)malloc(q->attrName.size + 1); /*alloc-3 */
+ attr[i]->attrName = (char *)malloc(q->attrName.size); /*alloc-3
+ */
strncpy(attr[i]->attrName, (const char *)q->attrName.buf,
- q->attrName.size + 1);
- attr[i]->attrName[q->attrName.size] = 0; /*redundant. */
+ q->attrName.size);
+ attr[i]->attrName[q->attrName.size-1] = 0; /*redundant. */
attr[i]->attrValuesNumber = q->attrValuesNumber;
attr[i]->attrValueType = (SaImmValueTypeT)q->attrValueType;
if (q->attrValuesNumber) {
diff --git a/src/imm/immnd/ImmModel.cc b/src/imm/immnd/ImmModel.cc index
733eb5049..2d750040e 100644
--- a/src/imm/immnd/ImmModel.cc
+++ b/src/imm/immnd/ImmModel.cc
@@ -8153,7 +8153,7 @@ SaAisErrorT ImmModel::ccbObjectCreate(
*/
if (isObjectDnUsed) {
attrValues->n.attrName.buf = strdup(i4->first.c_str());
- attrValues->n.attrName.size = i4->first.size();
+ attrValues->n.attrName.size = i4->first.size() + 1;
attrValues->n.attrValueType = i4->second->mValueType;
}
--
2.17.1
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
