-----Original Message-----
From: Hans Nordebäck [mailto:hans.nordeb...@ericsson.com]
Sent: Wednesday, March 7, 2018 1:31 PM
To: Zoran Milinkovic <zoran.milinko...@ericsson.com>; Vu Minh Nguyen
<vu.m.ngu...@dektech.com.au>; ravisekhar.ko...@oracle.com
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [devel] [PATCH 1/1] imm: not allow creating reserved IMM class
names [#2771]
Hi,
regarding Zoran's comment below, one question, isn't this code
redundant, regarding divide/multiply:
size_t len = sizeof(default_reserved_names)/
sizeof(default_reserved_names[0]);
cb->reserved_class_names =
(char**)calloc(1, len * sizeof(char*) + 1);
instead:
cb->reserved_class_names =
(char**)calloc(1, sizeof(default_reserved_names) +
sizeof(char*));
?
/Hans
On 03/06/2018 04:35 PM, Zoran Milinkovic wrote:
Hi Vu,
There are few things that I have found
1. imma_om_api.cc does not need to be changed.
2.
There is a wrong calculation for a new allocated memory in
populate_reserved_class_names().
It is:
+ cb->reserved_class_names =
+ (char**)calloc(1, len * sizeof(char*) + 1);
... but it should be...
+ cb->reserved_class_names =
+ (char**)calloc(1, (len + 1) * sizeof(char*));
3.
Both is_regular_name() and is_valid_schema_name() can be written in
immnd_main.c file where they are used first. Also add function definitions to
the header immnd_init.h.
There is no need for a new files.
I would also prefix functions with "immnd_"
Ack from me with minor comments.
No need to send the patch for another review
Thanks,
Zoran
-----Original Message-----
From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au]
Sent: den 30 januari 2018 15:24
To: Zoran Milinkovic <zoran.milinko...@ericsson.com>;
ravisekhar.ko...@oracle.com
Cc: opensaf-devel@lists.sourceforge.net; Vu Minh Nguyen
<vu.m.ngu...@dektech.com.au>
Subject: [PATCH 1/1] imm: not allow creating reserved IMM class names
[#2771]
PBE will be restarted and will not be able to come up if user requests
creating IMM object class with same name of reserved ones.
This patch adds code to reject such request with
SA_AIS_ERR_INVALID_PARAM.
---
src/imm/Makefile.am | 2 +
src/imm/agent/imma_om_api.cc | 1 -
.../apitest/management/test_saImmOmClassCreate_2.c | 48 +++++++++++
src/imm/immnd/ImmModel.cc | 55 +-----------
src/imm/immnd/immnd.conf | 9 ++
src/imm/immnd/immnd_cb.h | 1 +
src/imm/immnd/immnd_common.c | 75 ++++++++++++++++
src/imm/immnd/immnd_common.h | 32 +++++++
src/imm/immnd/immnd_evt.c | 17 ++++
src/imm/immnd/immnd_main.c | 99
+++++++++++++++++++++-
10 files changed, 285 insertions(+), 54 deletions(-)
create mode 100644 src/imm/immnd/immnd_common.c
create mode 100644 src/imm/immnd/immnd_common.h
diff --git a/src/imm/Makefile.am b/src/imm/Makefile.am
index b7e4826..099efda 100644
--- a/src/imm/Makefile.am
+++ b/src/imm/Makefile.am
@@ -163,6 +163,7 @@ noinst_HEADERS += \
src/imm/immnd/immnd.h \
src/imm/immnd/immnd_cb.h \
src/imm/immnd/immnd_init.h \
+ src/imm/immnd/immnd_common.h \
src/imm/immpbed/immpbe.h \
src/imm/tools/imm_dumper.h
@@ -332,6 +333,7 @@ bin_osafimmnd_CPPFLAGS = \
$(AM_CPPFLAGS)
bin_osafimmnd_SOURCES = \
+ src/imm/immnd/immnd_common.c \
src/imm/immnd/immnd_amf.c \
src/imm/immnd/immnd_db.c \
src/imm/immnd/immnd_evt.c \
diff --git a/src/imm/agent/imma_om_api.cc
b/src/imm/agent/imma_om_api.cc
index 7155799..a06f0ea 100644
--- a/src/imm/agent/imma_om_api.cc
+++ b/src/imm/agent/imma_om_api.cc
@@ -55,7 +55,6 @@ static bool immOmIsLoader = false;
static const char *sysaClName = SA_IMM_ATTR_CLASS_NAME;
static const char *sysaAdmName =
SA_IMM_ATTR_ADMIN_OWNER_NAME;
static const char *sysaImplName = SA_IMM_ATTR_IMPLEMENTER_NAME;
-
static int imma_om_resurrect(IMMA_CB *cb, IMMA_CLIENT_NODE
*cl_node,
bool *locked);
static SaAisErrorT imma_finalizeCcb(SaImmCcbHandleT ccbHandle,
diff --git a/src/imm/apitest/management/test_saImmOmClassCreate_2.c
b/src/imm/apitest/management/test_saImmOmClassCreate_2.c
index 3ae4b0f..967b819 100644
--- a/src/imm/apitest/management/test_saImmOmClassCreate_2.c
+++ b/src/imm/apitest/management/test_saImmOmClassCreate_2.c
@@ -426,6 +426,46 @@ void saImmOmClassCreate_2_19(void)
safassert(saImmOmFinalize(immOmHandle), SA_AIS_OK);
}
+/*
+ Verify it is not allowed to create IMM object class with reserved name.
+ NOTE: As the list of reserved class names is read from the environment
+ variable IMMSV_RESERVED_CLASS_NAMES which is defined in
immnd.conf file,
+ these 02 below test cases could fail if "objects" or "classes" name do
+ not exist in the list.
+ */
+void saImmOmClassCreate_with_reserved_name_01(void)
+{
+ const SaImmClassNameT className = (SaImmClassNameT) "objects";
+ SaImmAttrDefinitionT_2 attr1 = {"rdn", SA_IMM_ATTR_SANAMET,
+ SA_IMM_ATTR_CONFIG |
SA_IMM_ATTR_RDN,
+ NULL};
+ const SaImmAttrDefinitionT_2 *attrDefinitions[] = {&attr1, NULL};
+
+ safassert(saImmOmInitialize(&immOmHandle, &immOmCallbacks,
&immVersion),
+ SA_AIS_OK);
+ rc = saImmOmClassCreate_2(immOmHandle, className,
SA_IMM_CLASS_CONFIG,
+ attrDefinitions);
+ test_validate(rc, SA_AIS_ERR_INVALID_PARAM);
+ safassert(saImmOmFinalize(immOmHandle), SA_AIS_OK);
+}
+
+void saImmOmClassCreate_with_reserved_name_02(void)
+{
+ const SaImmClassNameT className = (SaImmClassNameT) "classes";
+ SaImmAttrDefinitionT_2 attr1 = {
+ "rdn", SA_IMM_ATTR_SANAMET,
+ SA_IMM_ATTR_RUNTIME | SA_IMM_ATTR_RDN |
SA_IMM_ATTR_CACHED,
+ NULL};
+ const SaImmAttrDefinitionT_2 *attrDefinitions[] = {&attr1, NULL};
+
+ safassert(saImmOmInitialize(&immOmHandle, &immOmCallbacks,
&immVersion),
+ SA_AIS_OK);
+ rc = saImmOmClassCreate_2(immOmHandle, className,
SA_IMM_CLASS_RUNTIME,
+ attrDefinitions);
+ test_validate(rc, SA_AIS_ERR_INVALID_PARAM);
+ safassert(saImmOmFinalize(immOmHandle), SA_AIS_OK);
+}
+
#define OPENSAF_IMM_NOSTD_FLAG_PARAM "opensafImmNostdFlags"
#define OPENSAF_IMM_NOSTD_FLAG_ON 1
#define OPENSAF_IMM_NOSTD_FLAG_OFF 2
@@ -1457,6 +1497,14 @@ __attribute__((constructor)) static void
saImmOmInitialize_constructor(void)
test_case_add(
2, saImmOmClassCreate_2_19,
"saImmOmClassCreate_2 - SA_AIS_OK, Create a class that has
STRONG_DEFAULT flag without having default value");
+ test_case_add(
+ 2, saImmOmClassCreate_with_reserved_name_01,
+ "saImmOmClassCreate_2 - SA_AIS_ERR_INVALID_PARAM,"
+ "Create a config class with reserved name");
+ test_case_add(
+ 2, saImmOmClassCreate_with_reserved_name_02,
+ "saImmOmClassCreate_2 - SA_AIS_ERR_INVALID_PARAM,"
+ " Create a rt class with reserved name");
test_case_add(2, saImmOmClassDescriptionGet_2_01,
"saImmOmClassDescriptionGet_2 - SA_AIS_OK");
diff --git a/src/imm/immnd/ImmModel.cc b/src/imm/immnd/ImmModel.cc
index 7e27a83..5cd85f9 100644
--- a/src/imm/immnd/ImmModel.cc
+++ b/src/imm/immnd/ImmModel.cc
@@ -21,6 +21,7 @@
#include <algorithm>
#include <time.h>
+#include "imm/immnd/immnd_common.h"
#include "imm/immnd/ImmModel.h"
#include "imm/immnd/ImmAttrValue.h"
#include "imm/immnd/ImmSearchOp.h"
@@ -13355,61 +13356,11 @@ bool ImmModel::checkSubLevel(const
std::string& objectName, size_t rootStart) {
}
bool ImmModel::schemaNameCheck(const std::string& name) const {
- /* Dont allow some chars in class & attribute names that cause
- problems in sqlite. Each imm-class is mapped to several tables,
- but one table is named using the classname.
- */
- unsigned char chr;
- size_t pos;
- size_t len = name.length();
-
- if (name.empty()) {
- return false;
- }
-
- if (!nameCheck(name)) {
- return false;
- }
-
- for (pos = 0; pos < len; ++pos) {
- chr = name.at(pos);
- if (isalnum(chr) || (chr == 95)) {
- continue; /* _ */
- } else {
- LOG_NO("Bad class/attribute name: '%s' (%c): pos=%zu", name.c_str(),
chr,
- pos);
- return false;
- }
- }
-
- chr = name.at(0);
-
- if (isdigit(chr)) {
- LOG_NO("Bad class/attribute name starts with number: '%s' (%c):
pos=%u",
- name.c_str(), chr, 0);
- return false;
- }
-
- return true;
+ return is_valid_schema_name(name.c_str());
}
bool ImmModel::nameCheck(const std::string& name, bool strict) const {
- size_t pos;
- size_t len = name.length();
- unsigned char prev_chr = '\0';
-
- for (pos = 0; pos < len; ++pos) {
- unsigned char chr = name.at(pos);
-
- if ((((chr == ',') || (strict && (chr == '#'))) && (prev_chr == '\\')) ||
- (!isgraph(chr) && !(chr == '\0' && pos == len - 1))) {
- TRACE_5("Irregular name. string size:%zu isgraph(%c):%u, pos=%zu",
len,
- chr, isgraph(chr), pos);
- return false;
- }
- prev_chr = chr;
- }
- return true;
+ return is_regular_name(name.c_str(), strict);
}
bool ImmModel::nameToInternal(std::string& name) {
diff --git a/src/imm/immnd/immnd.conf b/src/imm/immnd/immnd.conf
index 97af792..a2c6d85 100644
--- a/src/imm/immnd/immnd.conf
+++ b/src/imm/immnd/immnd.conf
@@ -71,3 +71,12 @@ export IMMSV_ENV_HEALTHCHECK_KEY="Default"
# Uncomment the next line to enable info level logging
#args="--loglevel=info"
+
+# The list of reserved table names. Each table is separated by a comma.
+# Any attempt to create IMM object class with one of these names will
+# get SA_AIS_ERR_INVALID_PARAM error code.
+# The default list is used if this enviroment variable is not defined.
+# export IMMSV_RESERVED_CLASS_NAMES="attr_def, attr_dflt,
ccb_commits, classes,
+# objects, objects_blob_multi,
+# objects_int_multi, objects_real_multi,
+# objects_text_multi, pbe_rep_version";
diff --git a/src/imm/immnd/immnd_cb.h b/src/imm/immnd/immnd_cb.h
index 7614d27..802b365 100644
--- a/src/imm/immnd/immnd_cb.h
+++ b/src/imm/immnd/immnd_cb.h
@@ -170,6 +170,7 @@ typedef struct immnd_cb_tag {
struct timespec
mJobStart; // Start time for major server tasks like start, load,
sync.
char *mProgName; // The full path name of the immnd executable.
+ char **reserved_class_names; // List of class names are reserved for PBE
const char *mDir; // The directory where imm.xml & pbe files reside
const char *mFile; // The imm.xml file to start from
const char *mPbeFile; // Pbe feature is configured (IMMSV_PBE_FILE).
diff --git a/src/imm/immnd/immnd_common.c
b/src/imm/immnd/immnd_common.c
new file mode 100644
index 0000000..af80a46
--- /dev/null
+++ b/src/imm/immnd/immnd_common.c
@@ -0,0 +1,75 @@
+/* -*- OpenSAF -*-
+ *
+ * Copyright Ericsson AB 2018 - 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
+ * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are
licensed
+ * under the GNU Lesser General Public License Version 2.1, February 1999.
+ * The complete license can be accessed from the following location:
+ * http://opensource.org/licenses/lgpl-license.php
+ * See the Copying file included with the OpenSAF distribution for full
+ * licensing terms.
+ *
+ */
+
+#include "imm/immnd/immnd_common.h"
+#include <ctype.h>
+#include <stdlib.h>
+#include <string.h>
+#include "base/logtrace.h"
+
+bool is_regular_name(const char* name, bool strict) {
+ size_t pos;
+ size_t len = strlen(name);
+ unsigned char prev_chr = '\0';
+
+ for (pos = 0; pos < len; ++pos) {
+ unsigned char chr = *(name + pos);
+
+ if ((((chr == ',') || (strict && (chr == '#')))
+ && (prev_chr == '\\')) ||
+ (!isgraph(chr) && !(chr == '\0' && pos == len - 1))) {
+ TRACE_5("Irregular name (%s). String size: %zu,"
+ " isgraph(%c): %u, pos = %zu",
+ name, len, chr, isgraph(chr), pos);
+ return false;
+ }
+ prev_chr = chr;
+ }
+ return true;
+}
+
+/*
+ Dont allow some chars in class & attribute names that cause
+ problems in sqlite. Each imm-class is mapped to several tables,
+ but one table is named using the classname.
+*/
+bool is_valid_schema_name(const char* name) {
+ unsigned char chr;
+ size_t pos;
+ size_t len = strlen(name);
+
+ if (len == 0) return false;
+ if (!is_regular_name(name, true)) return false;
+
+ for (pos = 0; pos < len; ++pos) {
+ chr = *(name + pos);
+ /* _ character */
+ if (isalnum(chr) || (chr == 95)) {
+ continue;
+ } else {
+ LOG_NO("Bad name: '%s' (%c): pos=%zu", name, chr,
pos);
+ return false;
+ }
+ }
+
+ chr = *name;
+ if (isdigit(chr)) {
+ LOG_NO("Bad name. Starts with number: '%s' (%c): pos=%u",
+ name, chr, 0);
+ return false;
+ }
+
+ return true;
+}
diff --git a/src/imm/immnd/immnd_common.h
b/src/imm/immnd/immnd_common.h
new file mode 100644
index 0000000..84241a8
--- /dev/null
+++ b/src/imm/immnd/immnd_common.h
@@ -0,0 +1,32 @@
+/* -*- OpenSAF -*-
+ *
+ * Copyright Ericsson AB 2018 - 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
+ * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are
licensed
+ * under the GNU Lesser General Public License Version 2.1, February 1999.
+ * The complete license can be accessed from the following location:
+ * http://opensource.org/licenses/lgpl-license.php
+ * See the Copying file included with the OpenSAF distribution for full
+ * licensing terms.
+ *
+ */
+
+#ifndef IMM_IMMND_IMMND_COMMON_H_
+#define IMM_IMMND_IMMND_COMMON_H_
+
+#include <stdbool.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+bool is_regular_name(const char* name, bool strict);
+bool is_valid_schema_name(const char* name);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif // IMM_IMMND_IMMND_COMMON_H_
diff --git a/src/imm/immnd/immnd_evt.c b/src/imm/immnd/immnd_evt.c
index b95dcdc..228b7dd 100644
--- a/src/imm/immnd/immnd_evt.c
+++ b/src/imm/immnd/immnd_evt.c
@@ -3592,6 +3592,17 @@ agent_rsp:
return rc;
}
+static bool is_class_name_reserved(const IMMND_CB* cb, const char*
name)
+{
+ int i = 0;
+ const char* r_name;
+ char** list = cb->reserved_class_names;
+ while ((r_name = list[i++])) {
+ if (!strcmp(name, r_name)) return true;
+ }
+ return false;
+}
+
/*
Function for performing immnd local checks on fevs packed messages.
Normally they pass the checks and are forwarded to the IMMD.
@@ -3864,6 +3875,12 @@ static SaAisErrorT
immnd_fevs_local_checks(IMMND_CB *cb, IMMSV_FEVS *fevsReq,
}
}
+ char *class_name =
+ frwrd_evt.info.immnd.info.classDescr.className.buf;
+ if (is_class_name_reserved(cb, class_name)) {
+ error = SA_AIS_ERR_INVALID_PARAM;
+ }
+
break;
case IMMND_EVT_A2ND_CLASS_DELETE:
diff --git a/src/imm/immnd/immnd_main.c
b/src/imm/immnd/immnd_main.c
index b68b374..5c801d7 100644
--- a/src/imm/immnd/immnd_main.c
+++ b/src/imm/immnd/immnd_main.c
@@ -36,7 +36,7 @@
#include "base/daemon.h"
#include "nid/agent/nid_api.h"
#include "mds/mds_dl_api.h"
-
+#include "imm/immnd/immnd_common.h"
#include "immnd.h"
#define FD_TERM 0
@@ -48,8 +48,103 @@
static IMMND_CB _immnd_cb;
IMMND_CB *immnd_cb = &_immnd_cb;
+static const char* default_reserved_names[] = {
+ "attr_def",
+ "attr_dflt",
+ "ccb_commits",
+ "classes",
+ "objects",
+ "objects_blob_multi",
+ "objects_int_multi",
+ "objects_real_multi",
+ "objects_text_multi",
+ "pbe_rep_version"
+};
+
/* Static Function Declerations */
+static char* trim_string(char* s)
+{
+ while (isspace((unsigned char) *s)) s++;
+ if (*s) {
+ char *p = s;
+ while (*p) p++;
+ while (isspace((unsigned char) *(--p)));
+ p[1] = '\0';
+ }
+ return s;
+}
+
+/*
+ Return the list of reserved class name if the input is valid, NULL otherwise.
+ */
+static char** parse_reserved_class_names(const char* input)
+{
+ char** result = NULL;
+ char* dup, *tofree;
+ char* token;
+ int i = 0, n_elements = 0;
+
+ dup = tofree = strdup(input);
+ token = strsep(&dup, ",");
+ while (token) {
+ token = trim_string(token);
+ if (is_valid_schema_name(token) == false) {
+ LOG_ER("The reserved name `%s` is invalid!", token);
+ goto freedata;
+ }
+ result = (char**)realloc(result, sizeof(char*) * ++n_elements);
+ result[n_elements - 1] = strdup(token);
+ token = strsep(&dup, ",");
+ }
+
+ result = (char**)realloc(result, sizeof(char*) * (n_elements+1));
+ result[n_elements] = 0;
+
+ free(tofree);
+ return result;
+
+freedata:
+ for (i = 0; i < n_elements; i++) {
+ if (result[i]) free(result[i]);
+ }
+ free(tofree);
+ free(result);
+ return NULL;
+}
+
+/*
+ Save list of reserved class names into global control block.
+ Use the default list if the environment variable
`IMMSV_RESERVED_CLASS_NAMES`
+ is not defined. If any invalid name exists, terminate IMMND and notify
+ the error to syslog.
+ */
+static void populate_reserved_class_names(IMMND_CB* cb)
+{
+ const char *envVar = NULL;
+ if ((envVar = getenv("IMMSV_RESERVED_CLASS_NAMES"))) {
+ cb->reserved_class_names =
parse_reserved_class_names(envVar);
+ /* IMMND is terminated if any wrong with the input value */
+ if (!cb->reserved_class_names) {
+ LOG_ER("The reserved class names are invalid! Exit...");
+ assert(0);
+ }
+ LOG_NO("The list of reserved class names: %s", envVar);
+ } else {
+ size_t i = 0;
+ size_t len = sizeof(default_reserved_names)/
+ sizeof(default_reserved_names[0]);
+ cb->reserved_class_names =
+ (char**)calloc(1, len * sizeof(char*) + 1);
+ for (; i < len; i++) {
+ cb->reserved_class_names[i] =
+ strdup(default_reserved_names[i]);
+ }
+ cb->reserved_class_names[i] = NULL;
+ LOG_NO("Use default reserved class names.");
+ }
+}
+
/**
* USR1 signal is used when AMF wants instantiate us as a
* component. Wake up the main thread so it can register with
@@ -150,6 +245,8 @@ static uint32_t immnd_initialize(char *progname)
immnd_cb->clm_hdl = 0;
immnd_cb->clmSelectionObject = -1;
+ populate_reserved_class_names(immnd_cb);
+
/* isClmNodeJoined will be intially set to true, untill CLMS service is
up. from there isClmNodeJoined will be controlled by CLM
membership
join/left.
