new patch attached

On 06/30/2015 03:37 PM, thierry bordaz wrote:
On 06/30/2015 12:07 PM, Ludwig Krispenz wrote:
added verification for issue reported in ticket 5088 and sanity checks requested in review for patch 0014


Hello,

The fix looks good except those sanity settings:

  * In ipa_topo_post_del, tsegm needs to be NULL initialized
  * In ipa_topo_check_segment_is_valid or ipa_topo_pre_add, I think
    *errtxt should be initialized to NULL

thanks
thierry


>From 042d11abe5b4ec9040bdf4c0709b74f46f5c38f5 Mon Sep 17 00:00:00 2001
From: Ludwig Krispenz <lkris...@redhat.com>
Date: Tue, 30 Jun 2015 16:02:16 +0200
Subject: [PATCH] v2 improve processing of invalid data.

    reject attempts to add segments to suffixes, which do not exist or are not configured.
    check completenes and validity of segment attributes

    cf ticket 5088: https://fedorahosted.org/freeipa/ticket/5088
---
 daemons/ipa-slapi-plugins/topology/topology_post.c | 11 ++++--
 daemons/ipa-slapi-plugins/topology/topology_pre.c  | 40 +++++++++++++++++-----
 daemons/ipa-slapi-plugins/topology/topology_util.c |  6 ++--
 3 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/topology/topology_post.c b/daemons/ipa-slapi-plugins/topology/topology_post.c
index c01505bdedc1b09488b21561315c721dc3847969..4eb3c2fd10643658804943112c1466091f9f624e 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_post.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_post.c
@@ -66,9 +66,14 @@ ipa_topo_post_add(Slapi_PBlock *pb)
         /* initialize the shared topology data for a replica */
         break;
     case TOPO_SEGMENT_ENTRY: {
-        TopoReplicaSegment *tsegm;
+        TopoReplicaSegment *tsegm = NULL;
         TopoReplica *tconf = ipa_topo_util_get_conf_for_segment(add_entry);
         char *status;
+        if (tconf == NULL) {
+            slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
+                            "ipa_topo_post_add - config area for segment not found\n");
+            break;
+        }
         /* TBD check that one node is the current server and
          * that the other node is also managed by the
          * shared config.
@@ -225,9 +230,9 @@ ipa_topo_post_del(Slapi_PBlock *pb)
     case TOPO_SEGMENT_ENTRY: {
         /* check if corresponding agreement exists and delete */
         TopoReplica *tconf = ipa_topo_util_get_conf_for_segment(del_entry);
-        TopoReplicaSegment *tsegm;
+        TopoReplicaSegment *tsegm = NULL;
         char *status;
-        tsegm = ipa_topo_util_find_segment(tconf, del_entry);
+        if (tconf) tsegm = ipa_topo_util_find_segment(tconf, del_entry);
         if (tsegm == NULL) {
             slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
                             "segment to be deleted does not exist\n");
diff --git a/daemons/ipa-slapi-plugins/topology/topology_pre.c b/daemons/ipa-slapi-plugins/topology/topology_pre.c
index c324cf69ed8396431a3171aefa5a02fc9e1dd1d9..0b9f8900940eda4429bb26fe6fc8118e2a71932b 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_pre.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_pre.c
@@ -284,7 +284,7 @@ ipa_topo_connection_exists(struct node_fanout *fanout, char* from, char *to)
 }
 
 int
-ipa_topo_check_segment_is_valid(Slapi_PBlock *pb)
+ipa_topo_check_segment_is_valid(Slapi_PBlock *pb, char **errtxt)
 {
     int rc = 0;
     Slapi_Entry *add_entry;
@@ -307,20 +307,38 @@ ipa_topo_check_segment_is_valid(Slapi_PBlock *pb)
         char *leftnode = slapi_entry_attr_get_charptr(add_entry,"ipaReplTopoSegmentLeftNode");
         char *rightnode = slapi_entry_attr_get_charptr(add_entry,"ipaReplTopoSegmentRightNode");
         char *dir = slapi_entry_attr_get_charptr(add_entry,"ipaReplTopoSegmentDirection");
-        if (strcasecmp(dir,SEGMENT_DIR_BOTH) && strcasecmp(dir,SEGMENT_DIR_LEFT_ORIGIN) &&
+        if (leftnode == NULL || rightnode == NULL || dir == NULL) {
+                *errtxt = slapi_ch_smprintf("Segment definition is incomplete"
+                                   ". Add rejected.\n");
+            rc = 1;
+        } else if (strcasecmp(dir,SEGMENT_DIR_BOTH) && strcasecmp(dir,SEGMENT_DIR_LEFT_ORIGIN) &&
             strcasecmp(dir,SEGMENT_DIR_RIGHT_ORIGIN)) {
+                *errtxt = slapi_ch_smprintf("Segment has unsupported direction"
+                                   ". Add rejected.\n");
                 slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
                                 "segment has unknown direction: %s\n", dir);
                 rc = 1;
         } else if (0 == strcasecmp(leftnode,rightnode)) {
+                *errtxt = slapi_ch_smprintf("Segment is self referential"
+                                   ". Add rejected.\n");
                 slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
                                 "segment is self referential\n");
                 rc = 1;
         } else {
-            TopoReplicaSegment *tsegm;
+            TopoReplicaSegment *tsegm = NULL;
             TopoReplica *tconf = ipa_topo_util_get_conf_for_segment(add_entry);
-            tsegm = ipa_topo_util_find_segment(tconf, add_entry);
+            if (tconf == NULL ) {
+                *errtxt = slapi_ch_smprintf("Segment configuration suffix not found"
+                                   ". Add rejected.\n");
+                slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
+                                "topology not configured for segment\n");
+                rc = 1;
+            } else {
+                tsegm = ipa_topo_util_find_segment(tconf, add_entry);
+            }
             if (tsegm) {
+                *errtxt = slapi_ch_smprintf("Segment already exists in topology"
+                                   ". Add rejected.\n");
                 slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
                                 "segment to be added does already exist\n");
                 rc = 1;
@@ -375,7 +393,13 @@ ipa_topo_check_topology_disconnect(Slapi_PBlock *pb)
         return 0;
     } else {
         TopoReplica *tconf = ipa_topo_util_get_conf_for_segment(del_entry);
-        TopoReplicaSegment *tsegm;
+        if (tconf == NULL) {
+            slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
+                            "topology not configured for segment\n");
+            rc = 0; /* this segment is not controlled by the plugin */
+            goto done;
+        }
+        TopoReplicaSegment *tsegm = NULL;
         tsegm = ipa_topo_util_find_segment(tconf, del_entry);
         if (tsegm == NULL) {
             slapi_log_error(SLAPI_LOG_FATAL, IPA_TOPO_PLUGIN_SUBSYSTEM,
@@ -412,6 +436,7 @@ ipa_topo_pre_ignore_op(Slapi_PBlock *pb)
 int ipa_topo_pre_add(Slapi_PBlock *pb)
 {
     int result = SLAPI_PLUGIN_SUCCESS;
+    char *errtxt  = NULL;
 
     slapi_log_error(SLAPI_LOG_PLUGIN, IPA_TOPO_PLUGIN_SUBSYSTEM,
                     "--> ipa_topo_pre_add\n");
@@ -432,11 +457,8 @@ int ipa_topo_pre_add(Slapi_PBlock *pb)
         slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, errtxt);
         slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);
         result = SLAPI_PLUGIN_FAILURE;
-    } else if (ipa_topo_check_segment_is_valid(pb)) {
+    } else if (ipa_topo_check_segment_is_valid(pb, &errtxt)) {
         int rc = LDAP_UNWILLING_TO_PERFORM;
-        char *errtxt;
-        errtxt = slapi_ch_smprintf("Segment already exists in topology or"
-                                   " is self referential. Add rejected.\n");
         slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, errtxt);
         slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc);
         result = SLAPI_PLUGIN_FAILURE;
diff --git a/daemons/ipa-slapi-plugins/topology/topology_util.c b/daemons/ipa-slapi-plugins/topology/topology_util.c
index a56704f51a282ebaa6dd86b80c1602689550f40f..fa249b96aa7b963e87272beb92c53e636263c590 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_util.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_util.c
@@ -448,8 +448,10 @@ ipa_topo_util_get_conf_for_segment(Slapi_Entry *segment_entry)
     char *parent = slapi_dn_parent(slapi_entry_get_dn_const(segment_entry));
 
     Slapi_Entry *conf = ipa_topo_util_get_entry(parent);
-    tconf = ipa_topo_util_conf_from_entry(conf);
-    slapi_entry_free(conf);
+    if (conf) {
+        tconf = ipa_topo_util_conf_from_entry(conf);
+        slapi_entry_free(conf);
+    }
 
     return tconf;
 }
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to