What: mca_base_param provides a deregister function. In the trunk this function
is only used in a ft path:
ompi-trunk hjelmn$ find . -name \*.[ch] | xargs grep mca_base_param_deregister
./ompi/mca/bml/r2/bml_r2_ft.c: mca_base_param_deregister(param_type);
Right now this function is broken for two reasons:
1) LEAK: The function does not destruct the mca_base_param_t it is
deregistering.
2) It removes the parameter from the array and moves every parameter after it
up by one index. This will break any code which stores and later uses the
parameter index! This isn't a big problem now but will
become one as MPI_T functionality is added to Open MPI.
The attached patch does the following:
- modify mca_base_param_deregister to correctly clean up the deleted parameter,
- modify the mca_base_param_t destructor to set the type to
MCA_BASE_PARAM_TYPE_MAX, and
- added parameter validity checks where they were missing (an invalid
parameter has type MCA_BASE_PARAM_TYPE_MAX.
Why: If MCA is to support deregistering mca parameters (I don't see why it
shouldn't) it needs to be done correctly.
When: This patch is not critical but I would like feedback on it soon. Timeout
set for next Monday (Nov. 26 @ 12:00PM MST).
Background: This is another part of a larger cleanup of the MCA system in
preperation for supporting the MPI tool interface specified in MPI-3.0. In this
case mca has a function that can change parameter ind
ices. This is not allowed by the MPI_T standard.
Please review the patch for correctness, bugs, etc.
-Nathan Hjelm
HPC-3, LANL
Index: opal/mca/base/mca_base_param.c
===================================================================
--- opal/mca/base/mca_base_param.c (revision 27624)
+++ opal/mca/base/mca_base_param.c (working copy)
@@ -469,15 +469,23 @@
*/
int mca_base_param_deregister(int index)
{
+ mca_base_param_t *array;
size_t size;
- /* Lookup the index and see if it's valid */
+ /* Lookup the index and see if the index and parameter are valid */
size = opal_value_array_get_size(&mca_base_params);
- if (index < 0 || ((size_t) index) > size) {
+ array = OPAL_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t);
+ if (index < 0 || ((size_t) index) > size ||
+ MCA_BASE_PARAM_TYPE_MAX >= array[index].mbp_type) {
return OPAL_ERROR;
}
- return opal_value_array_remove_item(&mca_base_params, index);
+ /* Do not remove this item from the array otherwise we will change
+ all the indices of parameters at a larger index. The destructor
+ will mark this parameter as invalid. */
+ OBJ_DESTRUCT(&array[index]);
+
+ return OPAL_SUCCESS;
}
/*
@@ -541,15 +549,13 @@
}
len = opal_value_array_get_size(&mca_base_params);
- if (index < 0 || ((size_t) index) > len) {
+ array = OPAL_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t);
+ if (index < 0 || ((size_t) index) > len ||
+ MCA_BASE_PARAM_TYPE_MAX <= array[index].mbp_type) {
return OPAL_ERROR;
}
- /* We have a valid entry (remember that we never delete MCA
- parameters, so if the index is >0 and <len, it must be good),
- so save the internal flag */
-
- array = OPAL_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t);
+ /* We have a valid entry so save the internal flag */
if (array[index].mbp_override_value_set) {
if (MCA_BASE_PARAM_TYPE_STRING == array[index].mbp_type &&
NULL != array[index].mbp_override_value.stringval) {
@@ -671,7 +677,8 @@
len = opal_value_array_get_size(&mca_base_params);
array = OPAL_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t);
for (i = 0; i < len; ++i) {
- if (array[i].mbp_internal == internal || internal) {
+ if ((array[i].mbp_internal == internal || internal) &&
+ MCA_BASE_PARAM_TYPE_MAX > array[i].mbp_type) {
p = OBJ_NEW(mca_base_param_info_t);
if (NULL == p) {
return OPAL_ERR_OUT_OF_RESOURCE;
@@ -835,19 +842,19 @@
mca_base_param_t *array;
if (initialized) {
+ int size, i;
- /* This is slow, but effective :-) */
-
+ size = opal_value_array_get_size(&mca_base_params);
array = OPAL_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t);
- while (opal_value_array_get_size(&mca_base_params) > 0) {
- OBJ_DESTRUCT(&array[0]);
- opal_value_array_remove_item(&mca_base_params, 0);
+ for (i = 0 ; i < size ; ++i) {
+ if (MCA_BASE_PARAM_TYPE_MAX > array[i].mbp_type) {
+ OBJ_DESTRUCT(&array[i]);
+ }
}
OBJ_DESTRUCT(&mca_base_params);
- for (item = opal_list_remove_first(&mca_base_param_file_values);
- NULL != item;
- item = opal_list_remove_first(&mca_base_param_file_values)) {
+ while (NULL !=
+ (item = opal_list_remove_first(&mca_base_param_file_values))) {
OBJ_RELEASE(item);
}
OBJ_DESTRUCT(&mca_base_param_file_values);
@@ -1504,6 +1511,13 @@
/* Find the param entry; add this syn_info to its list of
synonyms */
array = OPAL_VALUE_ARRAY_GET_BASE(&mca_base_params, mca_base_param_t);
+
+ /* Sanity check. Is this a valid parameter? */
+ if (MCA_BASE_PARAM_TYPE_MAX <= array[index_orig].mbp_type) {
+ OBJ_RELEASE(si);
+ return OPAL_ERROR;
+ }
+
if (NULL == array[index_orig].mbp_synonyms) {
array[index_orig].mbp_synonyms = OBJ_NEW(opal_list_t);
}
@@ -1545,7 +1559,10 @@
} else {
array[index].mbp_override_value.stringval = NULL;
}
+ } else {
+ return false;
}
+
array[index].mbp_override_value_set = true;
return true;
@@ -1995,6 +2012,9 @@
OBJ_RELEASE(p->mbp_synonyms);
}
+ /* mark this parameter as invalid */
+ p->mbp_type = MCA_BASE_PARAM_TYPE_MAX;
+
#if OPAL_ENABLE_DEBUG
/* Cheap trick to reset everything to NULL */
param_constructor(p);
