>From 57b00546ef860cf5c4e4ac47433d3d158ef78806 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 8 Apr 2016 13:58:52 +1200 Subject: [PATCH 1/2] VLV: handle empty results correctly The VLV was wrongly returning an operations error when the list of results was empty. Signed-off-by: Douglas Bagnall Pair-programmed-with: Garming Sam Reviewed-by: Garming Sam --- source4/dsdb/samdb/ldb_modules/vlv_pagination.c | 99 +++++++++++---------- source4/dsdb/tests/python/vlv.py | 112 ++++++++++++++++++++++-- 2 files changed, 162 insertions(+), 49 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c index fce705f..cc0d483 100644 --- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c +++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c @@ -355,48 +355,57 @@ static int vlv_results(struct vlv_context *ac) vlv_details = ac->store->vlv_details; sort_details = ac->store->sort_details; - if (vlv_details->type == 1) { - target = vlv_gt_eq_to_index(ac, ac->store->results, vlv_details, - sort_details, &ret); - if (ret != LDB_SUCCESS) { - return ret; - } - } else { - target = vlv_calc_real_offset(vlv_details->match.byOffset.offset, - vlv_details->match.byOffset.contentCount, - ac->store->num_entries); - if (target == -1) { - return LDB_ERR_OPERATIONS_ERROR; - } - } - /* send the results */ - first_i = MAX(target - vlv_details->beforeCount, 0); - last_i = MIN(target + vlv_details->afterCount, ac->store->num_entries - 1); - - for (i = first_i; i <= last_i; i++) { - struct ldb_result *result; - struct GUID *guid = &ac->store->results[i]; - ret = dsdb_search_by_dn_guid(ldb, ac, - &result, - guid, - ac->req->op.search.attrs, - 0); - - if (ret == LDAP_NO_SUCH_OBJECT) { - /* The thing isn't there, which we quietly ignore and - go on to send an extra one instead. */ - if (last_i < ac->store->num_entries - 1) { - last_i++; + if (ac->store->num_entries != 0) { + if (vlv_details->type == 1) { + target = vlv_gt_eq_to_index(ac, ac->store->results, + vlv_details, + sort_details, &ret); + if (ret != LDB_SUCCESS) { + return ret; + } + } else { + target = vlv_calc_real_offset(vlv_details->match.byOffset.offset, + vlv_details->match.byOffset.contentCount, + ac->store->num_entries); + if (target == -1) { + return LDB_ERR_OPERATIONS_ERROR; } - continue; - } else if (ret != LDB_SUCCESS) { - return ret; } - ret = ldb_module_send_entry(ac->req, result->msgs[0], NULL); - if (ret != LDB_SUCCESS) { - return ret; + /* send the results */ + first_i = MAX(target - vlv_details->beforeCount, 0); + last_i = MIN(target + vlv_details->afterCount, + ac->store->num_entries - 1); + + for (i = first_i; i <= last_i; i++) { + struct ldb_result *result; + struct GUID *guid = &ac->store->results[i]; + ret = dsdb_search_by_dn_guid(ldb, ac, + &result, + guid, + ac->req->op.search.attrs, + 0); + + if (ret == LDAP_NO_SUCH_OBJECT) { + /* The thing isn't there, which we quietly + ignore and go on to send an extra one + instead. */ + if (last_i < ac->store->num_entries - 1) { + last_i++; + } + continue; + } else if (ret != LDB_SUCCESS) { + return ret; + } + + ret = ldb_module_send_entry(ac->req, result->msgs[0], + NULL); + if (ret != LDB_SUCCESS) { + return ret; + } } + } else { + target = -1; } /* return result done */ @@ -512,12 +521,14 @@ static int vlv_search_callback(struct ldb_request *req, struct ldb_reply *ares) break; case LDB_REPLY_DONE: - store->results = talloc_realloc(store, store->results, - struct GUID, - store->num_entries); - if (store->results == NULL) { - return ldb_module_done(ac->req, NULL, NULL, - LDB_ERR_OPERATIONS_ERROR); + if (store->num_entries != 0) { + store->results = talloc_realloc(store, store->results, + struct GUID, + store->num_entries); + if (store->results == NULL) { + return ldb_module_done(ac->req, NULL, NULL, + LDB_ERR_OPERATIONS_ERROR); + } } store->result_array_size = store->num_entries; diff --git a/source4/dsdb/tests/python/vlv.py b/source4/dsdb/tests/python/vlv.py index 1db28c3..0283159 100644 --- a/source4/dsdb/tests/python/vlv.py +++ b/source4/dsdb/tests/python/vlv.py @@ -212,11 +212,12 @@ class VLVTests(samba.tests.TestCase): controls = res.controls return full_results, controls, sort_control - def get_expected_order(self, attr): + def get_expected_order(self, attr, expression=None): """Fetch the whole list sorted on the attribute, using sort only.""" sort_control = "server_sort:1:0:%s" % attr res = self.ldb.search(self.ou, scope=ldb.SCOPE_ONELEVEL, + expression=expression, attrs=[attr], controls=[sort_control]) results = [x[attr][0] for x in res] @@ -226,8 +227,8 @@ class VLVTests(samba.tests.TestCase): self.ldb.delete(user['dn']) del self.users[self.users.index(user)] - def get_gte_tests_and_order(self, attr): - expected_order = self.get_expected_order(attr) + def get_gte_tests_and_order(self, attr, expression=None): + expected_order = self.get_expected_order(attr, expression=expression) gte_users = [] if attr in self.delicate_keys: gte_keys = [ @@ -260,8 +261,10 @@ class VLVTests(samba.tests.TestCase): '\n\t\t', '桑巴', 'zzzz', - expected_order[len(expected_order) // 2] + ' tail', ] + if expected_order: + gte_keys.append(expected_order[len(expected_order) // 2] + ' tail') + else: # "numeric" means positive integers # doesn't work with -1, 3.14, ' 3', '9' * 20 @@ -285,7 +288,7 @@ class VLVTests(samba.tests.TestCase): self.delete_user(user) # for sanity's sake - expected_order_2 = self.get_expected_order(attr) + expected_order_2 = self.get_expected_order(attr, expression=expression) self.assertEqual(expected_order, expected_order_2) # Map gte tests to indexes in expected order. This will break @@ -377,6 +380,105 @@ class VLVTests(samba.tests.TestCase): self.assertCorrectResults(results, expected_order, offset, before, after) + def run_index_tests_with_expressions(self, expressions): + # Here we don't test every before/after combination. + attrs = [x for x in self.users[0].keys() if x not in + ('dn', 'objectclass')] + for attr in attrs: + for expression in expressions: + expected_order = self.get_expected_order(attr, expression) + sort_control = "server_sort:1:0:%s" % attr + res = None + n = len(expected_order) + for before in range(0, 11): + after = before + for offset in range(max(1, before - 2), + min(n - after + 2, n)): + if res is None: + vlv_search = "vlv:1:%d:%d:%d:0" % (before, after, + offset) + else: + cookie = get_cookie(res.controls) + vlv_search = ("vlv:1:%d:%d:%d:%s:%s" % + (before, after, offset, n, + cookie)) + + res = self.ldb.search(self.ou, + expression=expression, + scope=ldb.SCOPE_ONELEVEL, + attrs=[attr], + controls=[sort_control, + vlv_search]) + + results = [x[attr][0] for x in res] + + self.assertCorrectResults(results, expected_order, + offset, before, after) + + def test_server_vlv_with_failing_expression(self): + """What happens when we run the VLV on an expression that matches + nothing?""" + expressions = ["(samaccountname=testferf)", + "(cn=hefalump)", + ] + self.run_index_tests_with_expressions(expressions) + + def run_gte_tests_with_expressions(self, expressions): + # Here we don't test every before/after combination. + attrs = [x for x in self.users[0].keys() if x not in + ('dn', 'objectclass')] + for expression in expressions: + for attr in attrs: + gte_order, expected_order, gte_map = \ + self.get_gte_tests_and_order(attr, expression) + # In case there is some order dependency, disorder tests + gte_tests = gte_order[:] + random.seed(2) + random.shuffle(gte_tests) + res = None + sort_control = "server_sort:1:0:%s" % attr + + expected_order = self.get_expected_order(attr, expression) + sort_control = "server_sort:1:0:%s" % attr + res = None + for before in range(0, 11): + after = before + for gte in gte_tests: + if res is not None: + cookie = get_cookie(res.controls) + else: + cookie = None + vlv_search = encode_vlv_control(before=before, + after=after, + gte=gte, + cookie=cookie) + + res = self.ldb.search(self.ou, + scope=ldb.SCOPE_ONELEVEL, + expression=expression, + attrs=[attr], + controls=[sort_control, + vlv_search]) + + results = [x[attr][0] for x in res] + offset = gte_map.get(gte, len(expected_order)) + + # here offset is 0-based + start = max(offset - before, 0) + end = offset + 1 + after + + expected_results = expected_order[start: end] + + self.assertEquals(expected_results, results) + + def test_vlv_gte_with_failing_expression(self): + """What happens when we run the VLV on an expression that matches + nothing?""" + expressions = ["(samaccountname=testferf)", + "(cn=hefalump)", + ] + self.run_gte_tests_with_expressions(expressions) + def test_server_vlv_with_cookie_while_adding_and_deleting(self): """What happens if we add or remove items in the middle of the VLV? -- 2.5.0 >From 24c9a822c1bee8b907a9fe7788a0690c5a74365b Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 8 Apr 2016 14:00:45 +1200 Subject: [PATCH 2/2] VLV: test using restrictive expressions This tests what happens with the VLV if the results are subject to an expression. Signed-off-by: Douglas Bagnall Reviewed-by: Garming Sam --- source4/dsdb/tests/python/vlv.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/source4/dsdb/tests/python/vlv.py b/source4/dsdb/tests/python/vlv.py index 0283159..660d5b6 100644 --- a/source4/dsdb/tests/python/vlv.py +++ b/source4/dsdb/tests/python/vlv.py @@ -415,6 +415,14 @@ class VLVTests(samba.tests.TestCase): self.assertCorrectResults(results, expected_order, offset, before, after) + def test_server_vlv_with_expression(self): + """What happens when we run the VLV with an expression?""" + expressions = ["(objectClass=*)", + "(cn=%s)" % self.users[-1]['cn'], + "(roomNumber=%s)" % self.users[0]['roomNumber'], + ] + self.run_index_tests_with_expressions(expressions) + def test_server_vlv_with_failing_expression(self): """What happens when we run the VLV on an expression that matches nothing?""" @@ -471,6 +479,14 @@ class VLVTests(samba.tests.TestCase): self.assertEquals(expected_results, results) + def test_vlv_gte_with_expression(self): + """What happens when we run the VLV with an expression?""" + expressions = ["(objectClass=*)", + "(cn=%s)" % self.users[-1]['cn'], + "(roomNumber=%s)" % self.users[0]['roomNumber'], + ] + self.run_gte_tests_with_expressions(expressions) + def test_vlv_gte_with_failing_expression(self): """What happens when we run the VLV on an expression that matches nothing?""" -- 2.5.0