From b95e076d7c3ada4cc6556aaec745d3f45464d71c Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 12:54:59 +1200 Subject: [PATCH 1/4] vlv: Use strcmp(), not strncmp() checking the NULL terminated control OIDs The end result is the same, as sizeof() includes the trailing NUL, but this avoids having to think about that. Signed-off-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364 --- source4/dsdb/samdb/ldb_modules/vlv_pagination.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c index 980177cb05e..31e64b4bd78 100644 --- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c +++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c @@ -682,8 +682,8 @@ vlv_copy_down_controls(TALLOC_CTX *mem_ctx, struct ldb_control **controls) if (control->oid == NULL) { break; } - if (strncmp(control->oid, LDB_CONTROL_VLV_REQ_OID, sizeof(LDB_CONTROL_VLV_REQ_OID)) == 0 || - strncmp(control->oid, LDB_CONTROL_SERVER_SORT_OID, sizeof(LDB_CONTROL_SERVER_SORT_OID)) == 0) { + if (strcmp(control->oid, LDB_CONTROL_VLV_REQ_OID) == 0 || + strcmp(control->oid, LDB_CONTROL_SERVER_SORT_OID) == 0) { continue; } new_controls[j] = talloc_steal(new_controls, control); -- 2.17.1 From cb667057fb71be7928270747ed63c598f15959ce Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 13:16:48 +1200 Subject: [PATCH 2/4] selftest: Add test to confirm VLV interaction with ASQ Tested against Windows 1709. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/asq-vlv | 1 + source4/dsdb/tests/python/asq.py | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 selftest/knownfail.d/asq-vlv diff --git a/selftest/knownfail.d/asq-vlv b/selftest/knownfail.d/asq-vlv new file mode 100644 index 00000000000..fb791993e90 --- /dev/null +++ b/selftest/knownfail.d/asq-vlv @@ -0,0 +1 @@ +^samba4.asq.python\(ad_dc_default\).__main__.ASQLDAPTest.test_asq_vlv\(ad_dc_default\) \ No newline at end of file diff --git a/source4/dsdb/tests/python/asq.py b/source4/dsdb/tests/python/asq.py index a32c9f40cd3..a0c34031a18 100644 --- a/source4/dsdb/tests/python/asq.py +++ b/source4/dsdb/tests/python/asq.py @@ -162,6 +162,32 @@ class ASQLDAPTest(samba.tests.TestCase): self.assertIn(ldb.Dn(self.ldb, str(group)), self.members) + def test_asq_vlv(self): + """Testing ASQ behaviour with VLV set. + + ASQ is very strange, it turns a BASE search into a search for + all the objects pointed to by the specified attribute, + returning multiple entries! + + """ + + sort_control = "server_sort:1:0:cn" + + msgs = self.ldb.search(base=self.top_dn, + scope=ldb.SCOPE_BASE, + attrs=["objectGUID", "cn", "member"], + controls=["asq:1:member", + sort_control, + "vlv:1:20:20:11:0"]) + self.assertEqual(len(msgs), 20) + + for msg in msgs: + self.assertNotEqual(msg.dn, self.top_dn) + self.assertIn(msg.dn, self.members2) + for group in msg["member"]: + self.assertIn(ldb.Dn(self.ldb, str(group)), + self.members) + if "://" not in url: if os.path.isfile(url): url = "tdb://%s" % url -- 2.17.1 From 394c76e20912b8b0d4c249b15e98b67e23671b26 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 12:55:57 +1200 Subject: [PATCH 3/4] vlv: Do not re-ASQ search the results of an ASQ search with VLV This is a silly combination, but at least try and keep the results sensible and avoid a double-dereference. BUG: https://bugzilla.samba.org/show_bug.cgi?id=1436 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/asq-vlv | 1 - source4/dsdb/samdb/ldb_modules/vlv_pagination.c | 11 +++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) delete mode 100644 selftest/knownfail.d/asq-vlv diff --git a/selftest/knownfail.d/asq-vlv b/selftest/knownfail.d/asq-vlv deleted file mode 100644 index fb791993e90..00000000000 --- a/selftest/knownfail.d/asq-vlv +++ /dev/null @@ -1 +0,0 @@ -^samba4.asq.python\(ad_dc_default\).__main__.ASQLDAPTest.test_asq_vlv\(ad_dc_default\) \ No newline at end of file diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c index 31e64b4bd78..d58a62482c9 100644 --- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c +++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c @@ -682,10 +682,21 @@ vlv_copy_down_controls(TALLOC_CTX *mem_ctx, struct ldb_control **controls) if (control->oid == NULL) { break; } + /* + * Do not re-use VLV, nor the server-sort, both are + * already handled here. + */ if (strcmp(control->oid, LDB_CONTROL_VLV_REQ_OID) == 0 || strcmp(control->oid, LDB_CONTROL_SERVER_SORT_OID) == 0) { continue; } + /* + * ASQ changes everything, do not copy it down for the + * per-GUID search + */ + if (strcmp(control->oid, LDB_CONTROL_ASQ_OID) == 0) { + continue; + } new_controls[j] = talloc_steal(new_controls, control); j++; } -- 2.17.1 From df47f820336c15a5fc02611c6a72c07b769dce1a Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 16:34:11 +1200 Subject: [PATCH 4/4] vlv: Another workaround for mixing ASQ and VLV This is essentially an alternative patch, but without the correct behaviour. Instead this just avoids a segfault. Included in case we have something simialr again in another module. BUG: https://bugzilla.samba.org/show_bug.cgi?id=1436 Signed-off-by: Andrew Bartlett --- .../dsdb/samdb/ldb_modules/vlv_pagination.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c index d58a62482c9..720b5e95638 100644 --- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c +++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c @@ -442,10 +442,21 @@ static int vlv_results(struct vlv_context *ac) ret = vlv_search_by_dn_guid(ac->module, ac, &result, guid, ac->req->op.search.attrs); - 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 (ret == LDAP_NO_SUCH_OBJECT + || result->count != 1) { + /* + * The thing isn't there, which we quietly + * ignore and go on to send an extra one + * instead. + * + * result->count == 0 or > 1 can only + * happen if ASQ (which breaks all the + * rules) is somehow invoked (as this + * is a BASE search). + * + * (We skip the ASQ cookie for the + * GUID searches) + */ if (last_i < ac->store->num_entries - 1) { last_i++; } -- 2.17.1