[Freeipa-devel] [PATCHES 0172-0176] ipa_range_check improvements

[Alexander Bokovoy]

On Thu, 17 Apr 2014, Tomas Babej wrote:
>>From 43cd26a0a42c3b18e4dbb5c6ed0f20ee1562b98a Mon Sep 17 00:00:00 2001
>From: Tomas Babej <tbabej at redhat.com>
>Date: Wed, 16 Apr 2014 17:15:55 +0200
>Subject: [PATCH] ipa_range_check: Use special attributes to determine presence
> of RID bases
>
>The slapi_entry_attr_get_ulong which is used to get value of the RID base
>attributes returns 0 in case the attribute is not set at all. We need
>to distinguish this situation from the situation where RID base attributes
>are present, but deliberately set to 0.
>
>Otherwise this can cause false negative results of checks in the range_check
>plugin.
>
>Part of: https://fedorahosted.org/freeipa/ticket/4137
>---
> .../ipa-range-check/ipa_range_check.c              | 40 +++++++++++++++++-----
> 1 file changed, 31 insertions(+), 9 deletions(-)
>
>diff --git a/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c b/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
>index da5169e6e9bf74d5fbbf3aea40ee3e1a2c8f6016..68948f599aa4e6d21b071424ab27e3c62c0afefe 100644
>--- a/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
>+++ b/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
>@@ -88,6 +88,8 @@ struct range_info {
>     uint32_t id_range_size;
>     uint32_t base_rid;
>     uint32_t secondary_base_rid;
>+    bool base_rid_set;
>+    bool secondary_base_rid_set;
> };
> 
> static void free_range_info(struct range_info *range) {
>@@ -281,6 +283,7 @@ static int slapi_entry_to_range_info(struct domain_info *domain_info_head,
>     int ret;
>     unsigned long ul_val;
>     struct range_info *range = NULL;
>+    Slapi_Attr *attr;
> 
>     range = calloc(1, sizeof(struct range_info));
>     if (range == NULL) {
>@@ -326,6 +329,20 @@ static int slapi_entry_to_range_info(struct domain_info *domain_info_head,
>     }
>     range->secondary_base_rid = ul_val;
> 
>+    if (slapi_entry_attr_find(entry, IPA_BASE_RID, &attr) == -1) {
>+        range->base_rid_set = false;
>+    }
>+    else {
>+        range->base_rid_set = true;
>+    }
You replace this by
       range->base_rid_set = (slapi_entry_attr_find(entry, IPA_BASE_RID, &attr) == -1);

>+
>+    if (slapi_entry_attr_find(entry, IPA_SECONDARY_BASE_RID, &attr) == -1) {
>+        range->secondary_base_rid_set = false;
>+    }
>+    else {
>+        range->secondary_base_rid_set = true;
>+    }
>+
Same here.

>     *_range = range;
>     ret = 0;
> 
>@@ -398,12 +415,14 @@ static int check_ranges(struct range_info *r1, struct range_info *r2)
> 
>         /* For ipa-local or ipa-ad-trust range types primary RID ranges should
>          * not overlap */
>+
>         if (strcasecmp(r1->id_range_type, AD_TRUST_RANGE_TYPE) == 0 ||
>             strcasecmp(r1->id_range_type, LOCAL_RANGE_TYPE) == 0) {
> 
>-            /* Check if rid range overlaps with existing rid range */
>-            if (intervals_overlap(r1->base_rid, r2->base_rid,
>-                r1->id_range_size, r2->id_range_size))
>+            /* Check if primary rid range overlaps with existing primary rid range */
>+            if ((r1->base_rid_set && r2->base_rid_set) &&
>+                intervals_overlap(r1->base_rid, r2->base_rid,
>+                                  r1->id_range_size, r2->id_range_size))
>                 return 2;
>         }
> 
>@@ -412,18 +431,21 @@ static int check_ranges(struct range_info *r1, struct range_info *r2)
> 
>             /* Check if secondary RID range overlaps with existing secondary or
>              * primary RID range. */
>-            if (intervals_overlap(r1->secondary_base_rid,
>-                r2->secondary_base_rid, r1->id_range_size, r2->id_range_size))
>+            if ((r1->secondary_base_rid_set && r2->secondary_base_rid_set) &&
>+                intervals_overlap(r1->secondary_base_rid, r2->secondary_base_rid,
>+                                  r1->id_range_size, r2->id_range_size))
>                 return 3;
> 
>             /* Check if RID range overlaps with existing secondary RID range */
>-            if (intervals_overlap(r1->base_rid, r2->secondary_base_rid,
>-                r1->id_range_size, r2->id_range_size))
>+            if ((r1->base_rid_set && r2->secondary_base_rid_set) &&
>+                intervals_overlap(r1->base_rid, r2->secondary_base_rid,
>+                                  r1->id_range_size, r2->id_range_size))
>                 return 4;
> 
>             /* Check if secondary RID range overlaps with existing RID range */
>-            if (intervals_overlap(r1->secondary_base_rid, r2->base_rid,
>-                r1->id_range_size, r2->id_range_size))
>+            if ((r1->secondary_base_rid_set && r2->base_rid_set) &&
>+                intervals_overlap(r1->secondary_base_rid, r2->base_rid,
>+                                  r1->id_range_size, r2->id_range_size))
>                 return 5;
>             }
>     }
I know that is was in your original code, but can we get numbers
replaced by an enum? I'd prefer to see symbolic names used instead of
numbers.

-- 
/ Alexander Bokovoy