ack, only code review/BR HansN
-----Original Message-----
From: Gary Lee [mailto:[email protected]]
Sent: den 24 januari 2014 07:26
To: Hans Nordebäck; Gary Lee
Cc: [email protected]
Subject: [PATCH 1 of 1] amfd: fix out-of-bounds read from nodelist array [#719]
osaf/services/saf/amf/amfd/include/node.h | 1 +
osaf/services/saf/amf/amfd/nodegroup.cc | 35 ++++++++++++++++++++++++++++++-
osaf/services/saf/amf/amfd/sg.cc | 19 +---------------
3 files changed, 36 insertions(+), 19 deletions(-)
* fix out-of-bounds read in ng_ccb_apply_modify_hdlr() when the node to be
deleted is located at the end of saAmfNGNodeList
* modify ng_ccb_completed_modify_hdlr() to reject multiple node deletions
from a node group in a single CCB, as this is not currently supported
in ng_ccb_apply_modify_hdlr(). Also add a check for attempts to delete
a node that does not belong to the node group. Previously this caused
an assert.
* replace node_in_ng() from sg.cc with a similar function
node_in_nodegroup() in nodegroup.cc, to avoid code duplication.
diff --git a/osaf/services/saf/amf/amfd/include/node.h
b/osaf/services/saf/amf/amfd/include/node.h
--- a/osaf/services/saf/amf/amfd/include/node.h
+++ b/osaf/services/saf/amf/amfd/include/node.h
@@ -209,6 +209,7 @@
extern SaAisErrorT avd_ng_config_get(void); extern AVD_AMF_NG
*avd_ng_get(const SaNameT *dn); extern void avd_ng_constructor(void);
+extern bool node_in_nodegroup(const SaNameT *node, const AVD_AMF_NG
+*ng);
/* AMF Node SW Bundle */
extern void avd_nodeswbundle_constructor(void);
diff --git a/osaf/services/saf/amf/amfd/nodegroup.cc
b/osaf/services/saf/amf/amfd/nodegroup.cc
--- a/osaf/services/saf/amf/amfd/nodegroup.cc
+++ b/osaf/services/saf/amf/amfd/nodegroup.cc
@@ -260,6 +260,24 @@
}
/**
+ * Determine if a node is in a node group
+ * @param node node
+ * @param ng nodegroup
+ *
+ * @return true if found, otherwise false */ bool
+node_in_nodegroup(const SaNameT *node, const AVD_AMF_NG *ng) {
+ for (unsigned int i = 0; i < ng->number_nodes; i++) {
+ if ((ng->saAmfNGNodeList[i].length == node->length) &&
+ memcmp(&ng->saAmfNGNodeList[i].value, node->value,
node->length) == 0)
+ return true;
+ }
+
+ return false;
+}
+
+/**
* Validate modification of node group
* @param opdata
*
@@ -275,6 +293,7 @@
AVD_SU *su;
int delete_found = 0;
int add_found = 0;
+ int nodes_deleted = 0;
AVD_AMF_NG *ng;
TRACE_ENTER();
@@ -305,6 +324,12 @@
TRACE("DEL %s", ((SaNameT
*)mod->modAttr.attrValues[j])->value);
+ if (node_in_nodegroup((SaNameT
*)mod->modAttr.attrValues[j], ng) == false) {
+ report_ccb_validation_error(opdata, "ng
modify: node '%s' does not exist in node group",
+ ((SaNameT
*)mod->modAttr.attrValues[j])->value);
+ goto done;
+ }
+
/* Ensure no SU is mapped to this node via the
node group */
/* for all OpenSAF SUs hosted by this node */
@@ -327,6 +352,14 @@
goto done;
}
}
+
+ ++nodes_deleted;
+ /* currently, we don't support multiple node
deletions from a group in a CCB */
+ if (nodes_deleted > 1) {
+ report_ccb_validation_error(opdata,
+ "ng modify: cannot delete more
than one node from a node group in a CCB");
+ goto done;
+ }
}
}
@@ -509,7 +542,7 @@
TRACE("found node %s", ng->saAmfNGNodeList[j].value);
- for (; j < ng->number_nodes; j++)
+ for (; j < (ng->number_nodes - 1); j++)
ng->saAmfNGNodeList[j] = ng->saAmfNGNodeList[j
+ 1];
ng->number_nodes -= mod->modAttr.attrValuesNumber; diff
--git a/osaf/services/saf/amf/amfd/sg.cc b/osaf/services/saf/amf/amfd/sg.cc
--- a/osaf/services/saf/amf/amfd/sg.cc
+++ b/osaf/services/saf/amf/amfd/sg.cc
@@ -479,23 +479,6 @@
}
/**
- * Check if the node represented by nodename is a member of node group ng
- * @param nodename
- * @param ng
- * @return
- */
-static bool node_in_ng(const SaNameT *nodename, const AVD_AMF_NG *ng) -{
- for (unsigned i = 0; i < ng->number_nodes; i++) {
- if (strncmp((char*)nodename->value,
- (char*)ng->saAmfNGNodeList[i].value,
SA_MAX_NAME_LENGTH) == 0)
- return true;
- }
-
- return false;
-}
-
-/**
* Check if the node group represented by ngname is a subset of the node group
* represented by supername
* @param ngname
@@ -513,7 +496,7 @@
for (unsigned i = 0; i < ng->number_nodes; i++) {
nodename = &ng->saAmfNGNodeList[i];
- if (node_in_ng(nodename, superng) == false)
+ if (node_in_nodegroup(nodename, superng) == false)
return false;
}
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
