On 10/17/2012 11:14 AM, Sumit Bose wrote:
On Tue, Oct 16, 2012 at 02:26:24PM +0200, Tomas Babej wrote:
Hi,
commands ipa idrange-add / idrange-mod no longer allows the user
to enter primary or secondary rid range such that has non-zero
intersection with primary or secondary rid range of another
existing id range, as this could cause collision.
Unit tests added to test_range_plugin.py
https://fedorahosted.org/freeipa/ticket/3086
Tomas
Thank you for the patch, comments are in-line.
bye,
Sumit
>From a46a8d0aa4e64e105a53a177b6a12cf28e56620e Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Mon, 15 Oct 2012 06:28:16 -0400
Subject: [PATCH] Forbid overlapping primary and secondary rid ranges
Commands ipa idrange-add / idrange-mod no longer allows the user
to enter primary or secondary rid range such that has non-zero
intersection with primary or secondary rid range of another
existing id range, as this could cause collision.
Unit tests added to test_range_plugin.py
https://fedorahosted.org/freeipa/ticket/3086
---
.../ipa-range-check/ipa_range_check.c | 93 +++++++++++++---
tests/test_xmlrpc/test_range_plugin.py | 120 +++++++++++++++++++--
2 files changed, 191 insertions(+), 22 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
499e54a9c4a4c9134a231c0cd09e700390565a14..4f9f7437d11d2bc33238b14f5099a42b4c5463d2
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
@@ -132,24 +132,67 @@ done:
return ret;
}
-#define IN_RANGE(x,base,size) ( (x) >= (base) && ((x) - (base)) < (size) )
-static bool ranges_overlap(struct range_info *r1, struct range_info *r2)
+#define IN_RANGE(x,base,size) ( (x) >= (base) && (x) < (size+base) )
Would you mind to use the original definition of IN_RANGE? x-base looks
a bit odd, but I made it on purpose to avoid overruns. Since we already
know that x>=base we can be sure that there will be no underrun.
+static bool intervals_overlap(uint32_t x, uint32_t base, uint32_t x_size,
uint32_t base_size)
{
- if (r1->name != NULL && r2->name != NULL &&
- strcasecmp(r1->name, r2->name) == 0) {
- return false;
- }
-
- if (IN_RANGE(r1->base_id, r2->base_id, r2->id_range_size) ||
- IN_RANGE((r1->base_id + r1->id_range_size - 1), r2->base_id,
r2->id_range_size) ||
- IN_RANGE(r2->base_id, r1->base_id, r1->id_range_size) ||
- IN_RANGE((r2->base_id + r2->id_range_size - 1), r1->base_id,
r1->id_range_size)) {
+ if (IN_RANGE(x, base, base_size) ||
+ IN_RANGE((x + x_size - 1), base, base_size) ||
+ IN_RANGE(base, x, x_size) ||
+ IN_RANGE((base + base_size - 1), x, x_size)) {
return true;
}
return false;
}
+//returns 0 if there is no overlap
+//connected ranges must not overlap:
+// existing range: base rid sec_rid
+// | | \ / |
+// | | \/ |
+// | | /\ |
+// | | / \ |
+// new range: base rid sec_rid
I think we currently do not use C++ style comments in freeipa C code.
Can you switch to /* */ comments?
+static int ranges_overlap(struct range_info *r1, struct range_info *r2)
+{
+ if (r1->name != NULL && r2->name != NULL &&
+ strcasecmp(r1->name, r2->name) == 0) {
+ return 0;
+ }
+
+ //check if base range overlaps with existing base range
+ if (intervals_overlap(r1->base_id, r2->base_id,
+ r1->id_range_size, r2->id_range_size)){
+ return 1;
+ }
+
+ //if both base_rid and secondary_base_rid are 0, the rid range is not set
+ //in that case we skip the primary/secondary rid range overlap test
+ if((r1->base_rid!=0 || r1->secondary_base_rid!=0) &&
+ (r2->base_rid!=0 || r2->secondary_base_rid!=0)){
can you add spaces around '!=' ?
+
+ //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))
+ return 2;
+
+ //check if secondary rid range overlaps with existing secondary rid
range
+ if (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))
+ 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))
+ return 5;
+ }
Return code is missing if one of the ranges does not have the rid ranges
set. Can you add a test case for this condition as well?
+}
+
static int ipa_range_check_start(Slapi_PBlock *pb)
{
return 0;
@@ -176,7 +219,7 @@ static int ipa_range_check_pre_op(Slapi_PBlock *pb, int
modtype)
int search_result;
Slapi_Entry **search_entries = NULL;
size_t c;
- bool overlap = true;
+ int no_overlap = 0;
const char *check_attr;
char *errmsg = NULL;
@@ -315,13 +358,31 @@ static int ipa_range_check_pre_op(Slapi_PBlock *pb, int modtype)
goto done;
}
- overlap = ranges_overlap(old_range, new_range);
+ no_overlap = ranges_overlap(new_range, old_range);
free(old_range);
old_range = NULL;
- if (overlap) {
- LOG_FATAL("New range overlaps with existing one.\n");
+ if (no_overlap != 0) {
ret = LDAP_CONSTRAINT_VIOLATION;
- errmsg = "New range overlaps with existing one.";
+ if (no_overlap==1) {
+ errmsg = "New base range overlaps with existing base range.";
+ LOG_FATAL("New base range overlaps with existing base
range.\n");
+ }
+ if (no_overlap==2) {
+ errmsg = "New primary rid range overlaps with existing primary rid
range.";
+ LOG_FATAL("New primary rid range overlaps with existing primary rid
range.\n");
+ }
+ if (no_overlap==3) {
+ errmsg = "New secondary rid range overlaps with existing secondary
rid range.";
+ LOG_FATAL("New secondary rid range overlaps with existing secondary
rid range.\n");
+ }
+ if (no_overlap==4) {
+ errmsg = "New primary rid range overlaps with existing secondary
rid range.";
+ LOG_FATAL("New primary rid range overlaps with existing secondary
rid range.\n");
+ }
+ if (no_overlap==5) {
+ errmsg = "New secondary rid range overlaps with existing primary
rid range.";
+ LOG_FATAL("New secondary rid range overlaps with existing primary
rid range.\n");
+ }
Can you change this to a switch block? Please also add a default
section. This was new return codes (for new checks) can be added and we
still get a message even if it does not have a matching message here.
LOG_FATAL can handle arguments, so I would suggest to add a
LOG_FATAL("%s\n", errmsg);
after the switch block.
goto done;
}
}
Thank you for your suggestions. New version of the patch attached.
Tomas
>From 5b60d64df7ea62a50477e133e62ffd771946d1f0 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Mon, 15 Oct 2012 06:28:16 -0400
Subject: [PATCH] Forbid overlapping primary and secondary rid ranges
Commands ipa idrange-add / idrange-mod no longer allows the user
to enter primary or secondary rid range such that has non-zero
intersection with primary or secondary rid range of another
existing id range, as this could cause collision.
Unit tests added to test_range_plugin.py
https://fedorahosted.org/freeipa/ticket/3086
---
.../ipa-range-check/ipa_range_check.c | 101 ++++++++++++++---
tests/test_xmlrpc/test_range_plugin.py | 120 +++++++++++++++++++--
2 files changed, 199 insertions(+), 22 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 499e54a9c4a4c9134a231c0cd09e700390565a14..297ace49c73eb8af3ca41fa3f89cee6a98e52cfc 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
@@ -132,24 +132,72 @@ done:
return ret;
}
-#define IN_RANGE(x,base,size) ( (x) >= (base) && ((x) - (base)) < (size) )
-static bool ranges_overlap(struct range_info *r1, struct range_info *r2)
+#define IN_RANGE(x,base,size) ( (x) >= (base) && ((x) - (base) < (size)) )
+static bool intervals_overlap(uint32_t x, uint32_t base, uint32_t x_size, uint32_t base_size)
{
- if (r1->name != NULL && r2->name != NULL &&
- strcasecmp(r1->name, r2->name) == 0) {
- return false;
- }
-
- if (IN_RANGE(r1->base_id, r2->base_id, r2->id_range_size) ||
- IN_RANGE((r1->base_id + r1->id_range_size - 1), r2->base_id, r2->id_range_size) ||
- IN_RANGE(r2->base_id, r1->base_id, r1->id_range_size) ||
- IN_RANGE((r2->base_id + r2->id_range_size - 1), r1->base_id, r1->id_range_size)) {
+ if (IN_RANGE(x, base, base_size) ||
+ IN_RANGE((x + x_size - 1), base, base_size) ||
+ IN_RANGE(base, x, x_size) ||
+ IN_RANGE((base + base_size - 1), x, x_size)) {
return true;
}
return false;
}
+/**
+ * returns 0 if there is no overlap
+ *
+ * connected ranges must not overlap:
+ * existing range: base rid sec_rid
+ * | | \ / |
+ * | | \/ |
+ * | | /\ |
+ * | | / \ |
+ * new range: base rid sec_rid
+ */
+static int ranges_overlap(struct range_info *r1, struct range_info *r2)
+{
+ if (r1->name != NULL && r2->name != NULL &&
+ strcasecmp(r1->name, r2->name) == 0) {
+ return 0;
+ }
+
+ /* check if base range overlaps with existing base range */
+ if (intervals_overlap(r1->base_id, r2->base_id,
+ r1->id_range_size, r2->id_range_size)){
+ return 1;
+ }
+
+ /* if both base_rid and secondary_base_rid are 0, the rid range is not set
+ in that case we can skip the primary/secondary rid range overlap test */
+ if((r1->base_rid != 0 || r1->secondary_base_rid != 0) &&
+ (r2->base_rid != 0 || r2->secondary_base_rid != 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))
+ return 2;
+
+ /* check if secondary rid range overlaps with existing secondary rid range */
+ if (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))
+ 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))
+ return 5;
+ }
+
+ return 0;
+}
+
static int ipa_range_check_start(Slapi_PBlock *pb)
{
return 0;
@@ -176,7 +224,7 @@ static int ipa_range_check_pre_op(Slapi_PBlock *pb, int modtype)
int search_result;
Slapi_Entry **search_entries = NULL;
size_t c;
- bool overlap = true;
+ int no_overlap = 0;
const char *check_attr;
char *errmsg = NULL;
@@ -315,13 +363,34 @@ static int ipa_range_check_pre_op(Slapi_PBlock *pb, int modtype)
goto done;
}
- overlap = ranges_overlap(old_range, new_range);
+ no_overlap = ranges_overlap(new_range, old_range);
free(old_range);
old_range = NULL;
- if (overlap) {
- LOG_FATAL("New range overlaps with existing one.\n");
+ if (no_overlap != 0) {
ret = LDAP_CONSTRAINT_VIOLATION;
- errmsg = "New range overlaps with existing one.";
+
+ switch (no_overlap){
+ case 1:
+ errmsg = "New base range overlaps with existing base range.";
+ break;
+ case 2:
+ errmsg = "New primary rid range overlaps with existing primary rid range.";
+ break;
+ case 3:
+ errmsg = "New secondary rid range overlaps with existing secondary rid range.";
+ break;
+ case 4:
+ errmsg = "New primary rid range overlaps with existing secondary rid range.";
+ break;
+ case 5:
+ errmsg = "New secondary rid range overlaps with existing primary rid range.";
+ break;
+ default:
+ errmsg = "New range overlaps with existing one."
+ break;
+ }
+
+ LOG_FATAL("%s\n",errmsg);
goto done;
}
}
diff --git a/tests/test_xmlrpc/test_range_plugin.py b/tests/test_xmlrpc/test_range_plugin.py
index 00e430bf0458e40f622e731566de2ab760f49bf5..56c6db528b6c9cc4e1ac8cf22232fdfa46953389 100644
--- a/tests/test_xmlrpc/test_range_plugin.py
+++ b/tests/test_xmlrpc/test_range_plugin.py
@@ -27,10 +27,41 @@ from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid
from tests.test_xmlrpc import objectclasses
from ipapython.dn import *
-testrange1 = u't-range-1'
+testrange1 = u'testrange1'
testrange1_base_id = 900000
testrange1_size = 99999
+testrange2 = u'testrange2'
+testrange2_base_id = 100
+testrange2_size = 50
+testrange2_base_rid = 100
+testrange2_secondary_base_rid=1000
+
+testrange3 = u'testrange3'
+testrange3_base_id = 200
+testrange3_size = 50
+testrange3_base_rid = 70
+testrange3_secondary_base_rid=1100
+
+testrange4 = u'testrange4'
+testrange4_base_id = 300
+testrange4_size = 50
+testrange4_base_rid = 200
+testrange4_secondary_base_rid=1030
+
+testrange5 = u'testrange5'
+testrange5_base_id = 400
+testrange5_size = 50
+testrange5_base_rid = 1020
+testrange5_secondary_base_rid=1200
+
+testrange6 = u'testrange6'
+testrange6_base_id = 130
+testrange6_size = 50
+testrange6_base_rid = 500
+testrange6_secondary_base_rid=1300
+
+
user1=u'tuser1'
user1_uid = 900000
group1=u'group1'
@@ -38,7 +69,7 @@ group1_gid = 900100
class test_range(Declarative):
cleanup_commands = [
- ('idrange_del', [testrange1], {}),
+ ('idrange_del', [testrange1,testrange2,testrange3,testrange4,testrange5,testrange6], {}),
('user_del', [user1], {}),
('group_del', [group1], {}),
]
@@ -48,7 +79,7 @@ class test_range(Declarative):
desc='Create ID range %r' % (testrange1),
command=('idrange_add', [testrange1],
dict(ipabaseid=testrange1_base_id, ipaidrangesize=testrange1_size,
- ipabaserid=1000, ipasecondarybaserid=20000)),
+ ipabaserid=10000, ipasecondarybaserid=20000)),
expected=dict(
result=dict(
dn=DN(('cn',testrange1),('cn','ranges'),('cn','etc'),
@@ -56,7 +87,7 @@ class test_range(Declarative):
cn=[testrange1],
objectclass=[u'ipaIDrange', u'ipadomainidrange'],
ipabaseid=[unicode(testrange1_base_id)],
- ipabaserid=[u'1000'],
+ ipabaserid=[u'10000'],
ipasecondarybaserid=[u'20000'],
ipaidrangesize=[unicode(testrange1_size)],
iparangetype=[u'local domain range'],
@@ -75,7 +106,7 @@ class test_range(Declarative):
api.env.basedn),
cn=[testrange1],
ipabaseid=[unicode(testrange1_base_id)],
- ipabaserid=[u'1000'],
+ ipabaserid=[u'10000'],
ipasecondarybaserid=[u'20000'],
ipaidrangesize=[unicode(testrange1_size)],
iparangetype=[u'local domain range'],
@@ -179,7 +210,7 @@ class test_range(Declarative):
result=dict(
cn=[testrange1],
ipabaseid=[unicode(testrange1_base_id)],
- ipabaserid=[u'1000'],
+ ipabaserid=[u'10000'],
ipasecondarybaserid=[u'20000'],
ipaidrangesize=[u'90000'],
iparangetype=[u'local domain range'],
@@ -231,4 +262,81 @@ class test_range(Declarative):
),
),
+ dict(
+ desc='Create ID range %r' % (testrange2),
+ command=('idrange_add', [testrange2],
+ dict(ipabaseid=testrange2_base_id,
+ ipaidrangesize=testrange2_size,
+ ipabaserid=testrange2_base_rid,
+ ipasecondarybaserid=testrange2_secondary_base_rid)),
+ expected=dict(
+ result=dict(
+ dn=DN(('cn',testrange2),('cn','ranges'),('cn','etc'),
+ api.env.basedn),
+ cn=[testrange2],
+ objectclass=[u'ipaIDrange', u'ipadomainidrange'],
+ ipabaseid=[unicode(testrange2_base_id)],
+ ipabaserid=[unicode(testrange2_base_rid)],
+ ipasecondarybaserid=[unicode(testrange2_secondary_base_rid)],
+ ipaidrangesize=[unicode(testrange2_size)],
+ iparangetype=[u'local domain range'],
+ ),
+ value=testrange2,
+ summary=u'Added ID range "%s"' % (testrange2),
+ ),
+ ),
+
+ dict(
+ desc='Try to create ID range %r with overlapping rid range' % (testrange3),
+ command=('idrange_add', [testrange3],
+ dict(ipabaseid=testrange3_base_id,
+ ipaidrangesize=testrange3_size,
+ ipabaserid=testrange3_base_rid,
+ ipasecondarybaserid=testrange3_secondary_base_rid)),
+ expected=errors.DatabaseError(
+ desc='Constraint violation', info='New primary rid range overlaps with existing primary rid range.'),
+ ),
+
+ dict(
+ desc='Try to create ID range %r with overlapping secondary rid range' % (testrange4),
+ command=('idrange_add', [testrange4],
+ dict(ipabaseid=testrange4_base_id,
+ ipaidrangesize=testrange4_size,
+ ipabaserid=testrange4_base_rid,
+ ipasecondarybaserid=testrange4_secondary_base_rid)),
+ expected=errors.DatabaseError(
+ desc='Constraint violation', info='New secondary rid range overlaps with existing secondary rid range.'),
+ ),
+
+ dict(
+ desc='Try to create ID range %r with primary range overlapping secondary rid range' % (testrange5),
+ command=('idrange_add', [testrange5],
+ dict(ipabaseid=testrange5_base_id,
+ ipaidrangesize=testrange5_size,
+ ipabaserid=testrange5_base_rid,
+ ipasecondarybaserid=testrange5_secondary_base_rid)),
+ expected=errors.DatabaseError(
+ desc='Constraint violation', info='New primary rid range overlaps with existing secondary rid range.'),
+ ),
+
+ dict(
+ desc='Try to create ID range %r with overlapping id range' % (testrange6),
+ command=('idrange_add', [testrange6],
+ dict(ipabaseid=testrange6_base_id,
+ ipaidrangesize=testrange6_size,
+ ipabaserid=testrange6_base_rid,
+ ipasecondarybaserid=testrange6_secondary_base_rid)),
+ expected=errors.DatabaseError(
+ desc='Constraint violation', info='New base range overlaps with existing base range.'),
+ ),
+
+ dict(
+ desc='Delete ID range %r' % testrange2,
+ command=('idrange_del', [testrange2], {}),
+ expected=dict(
+ result=dict(failed=u''),
+ value=testrange2,
+ summary=u'Deleted ID range "%s"' % testrange2,
+ ),
+ ),
]
--
1.7.11.7
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
