From 04144ccd370eef9848101372a4061b10228da5de Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 18 Jan 2018 14:54:04 +0100 Subject: [PATCH 1/3] testprogs:blackbox: add regression test for unsorted links in tombstones-expunge.sh BUG: https://bugzilla.samba.org/show_bug.cgi?id=13228 Signed-off-by: Stefan Metzmacher Reviewed-by: Ralph Boehme (cherry picked from commit c34c2dd55545b99fba46cf374a1653bad96cea9e) --- selftest/knownfail.d/unsorted-links | 1 + .../add-unsorted-links-step1.ldif | 72 ++++++++++++++++++++++ .../add-unsorted-links-step2.ldif | 12 ++++ .../release-4-5-0-pre1/expected-expunge-output.txt | 2 +- .../expected-unsorted-links-after-expunge.ldif | 23 +++++++ testprogs/blackbox/tombstones-expunge.sh | 24 ++++++++ 6 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 selftest/knownfail.d/unsorted-links create mode 100644 source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step1.ldif create mode 100644 source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step2.ldif create mode 100644 source4/selftest/provisions/release-4-5-0-pre1/expected-unsorted-links-after-expunge.ldif diff --git a/selftest/knownfail.d/unsorted-links b/selftest/knownfail.d/unsorted-links new file mode 100644 index 0000000..0b19ae5 --- /dev/null +++ b/selftest/knownfail.d/unsorted-links @@ -0,0 +1 @@ +^samba4.blackbox.tombstones-expunge.release-4-5-0-pre1.check_expected_unsorted_links diff --git a/source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step1.ldif b/source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step1.ldif new file mode 100644 index 0000000..a1b9895 --- /dev/null +++ b/source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step1.ldif @@ -0,0 +1,72 @@ +dn: CN=unsorted-u1,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u1 +objectGUID: 9f92d30a-fc23-11e7-a5f6-37be15454801 + +dn: CN=unsorted-u2,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u2 +objectGUID: 9f92d30a-fc23-11e7-a5f6-37be15454802 + +dn: CN=unsorted-u3,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u3 +objectGUID: 9f92d30a-fc23-11e7-a5f6-30be15454803 + +dn: CN=unsorted-u4,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u4 +objectGUID: 9f92d30a-fc23-11e7-a5f6-30be15454804 + +dn: CN=unsorted-u5,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u5 +objectGUID: 9f92d30a-fc23-11e7-a5f6-30be15454805 + +dn: CN=unsorted-u6,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u6 +objectGUID: 9f92d30a-fc23-11e7-a5f6-30be15454806 + +dn: CN=unsorted-u7,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u7 +objectGUID: 9f92d30a-fc23-11e7-a5f6-30be15454807 + +dn: CN=unsorted-u8,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u8 +objectGUID: 9f92d30a-fc23-11e4-a5f6-30be15454808 + +dn: CN=unsorted-u9,CN=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: user +samaccountname: unsorted-u9 +objectGUID: 9f92d30a-fc23-11e4-a5f6-30be15454809 + +dn: cn=unsorted-g,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: add +objectclass: group +samaccountname: unsorted-g +member: cn=unsorted-u1,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u2,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u3,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u4,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u5,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u6,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u7,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u8,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: cn=unsorted-u9,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp + +dn: cn=unsorted-g,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: modify +delete: member +member: cn=unsorted-u5,cn=users,DC=release-4-5-0-pre1,DC=samba,DC=corp diff --git a/source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step2.ldif b/source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step2.ldif new file mode 100644 index 0000000..521b31e --- /dev/null +++ b/source4/selftest/provisions/release-4-5-0-pre1/add-unsorted-links-step2.ldif @@ -0,0 +1,12 @@ +dn: CN=unsorted-g,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +changetype: modify +replace: member +member: ;;;;;;;;;CN=unsorted-u1,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u2,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u3,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u4,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u5,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u6,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u7,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u8,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: ;;;;;;;;;CN=unsorted-u9,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp diff --git a/source4/selftest/provisions/release-4-5-0-pre1/expected-expunge-output.txt b/source4/selftest/provisions/release-4-5-0-pre1/expected-expunge-output.txt index 6826257..06a25a3 100644 --- a/source4/selftest/provisions/release-4-5-0-pre1/expected-expunge-output.txt +++ b/source4/selftest/provisions/release-4-5-0-pre1/expected-expunge-output.txt @@ -1 +1 @@ -Removed 7 objects and 2 links successfully +Removed 7 objects and 3 links successfully diff --git a/source4/selftest/provisions/release-4-5-0-pre1/expected-unsorted-links-after-expunge.ldif b/source4/selftest/provisions/release-4-5-0-pre1/expected-unsorted-links-after-expunge.ldif new file mode 100644 index 0000000..d078fd9 --- /dev/null +++ b/source4/selftest/provisions/release-4-5-0-pre1/expected-unsorted-links-after-expunge.ldif @@ -0,0 +1,23 @@ +# record 1 +dn: CN=unsorted-g,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u8,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u9,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u3,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u4,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u6,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u7,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u1,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp +member: CN=unsorted-u2,CN=Users,DC=release-4-5-0-pre1,DC=samba,DC=corp + +# Referral +ref: ldap:///CN=Configuration,DC=release-4-5-0-pre1,DC=samba,DC=corp + +# Referral +ref: ldap:///DC=DomainDnsZones,DC=release-4-5-0-pre1,DC=samba,DC=corp + +# Referral +ref: ldap:///DC=ForestDnsZones,DC=release-4-5-0-pre1,DC=samba,DC=corp + +# returned 4 records +# 1 entries +# 3 referrals diff --git a/testprogs/blackbox/tombstones-expunge.sh b/testprogs/blackbox/tombstones-expunge.sh index 3f7d708..d03547f 100755 --- a/testprogs/blackbox/tombstones-expunge.sh +++ b/testprogs/blackbox/tombstones-expunge.sh @@ -92,6 +92,19 @@ add_four_more_links() { fi } +add_unsorted_links() { + ldif=$release_dir/add-unsorted-links-step1.ldif + TZ=UTC $ldbmodify -H tdb://$PREFIX_ABS/${RELEASE}/private/sam.ldb $ldif --relax + if [ "$?" != "0" ]; then + return 1 + fi + ldif=$release_dir/add-unsorted-links-step2.ldif + TZ=UTC $ldbmodify -H tdb://$PREFIX_ABS/${RELEASE}/private/sam.ldb.d/DC%3DRELEASE-4-5-0-PRE1,DC%3DSAMBA,DC%3DCORP.ldb $ldif + if [ "$?" != "0" ]; then + return 1 + fi +} + remove_one_link() { ldif=$release_dir/remove-one-more-link.ldif TZ=UTC $ldbmodify -H tdb://$PREFIX_ABS/${RELEASE}/private/sam.ldb $ldif @@ -176,6 +189,15 @@ check_expected_after_objects() { fi } +check_expected_unsorted_links() { + tmpldif=$PREFIX_ABS/$RELEASE/expected-unsorted-links-after-expunge.ldif.tmp + TZ=UTC $ldbsearch -H tdb://$PREFIX_ABS/${RELEASE}/private/sam.ldb '(name=unsorted-g)' -s sub -b DC=release-4-5-0-pre1,DC=samba,DC=corp --show-deleted --reveal --sorted member > $tmpldif + diff $tmpldif $release_dir/expected-unsorted-links-after-expunge.ldif + if [ "$?" != "0" ]; then + return 1 + fi +} + if [ -d $release_dir ]; then testit $RELEASE undump testit "add_two_more_users" add_two_more_users @@ -192,10 +214,12 @@ if [ -d $release_dir ]; then testit_expect_failure "check_match_rule_links_decimal" check_match_rule_links_decimal testit_expect_failure "check_match_rule_links_backlink" check_match_rule_links_backlink testit_expect_failure "check_match_rule_links_notlink" check_match_rule_links_notlink + testit "add_unsorted_links" add_unsorted_links testit "tombstones_expunge" tombstones_expunge testit "check_expected_after_deleted_links" check_expected_after_deleted_links testit "check_expected_after_links" check_expected_after_links testit "check_expected_after_objects" check_expected_after_objects + testit "check_expected_unsorted_links" check_expected_unsorted_links else subunit_start_test $RELEASE subunit_skip_test $RELEASE < Date: Wed, 17 Jan 2018 08:07:03 +0100 Subject: [PATCH 2/3] repl_meta_data: fix linked attribute corruption on databases with unsorted links on expunge This is really critical bug, it removes valid linked attributes. When a DC was provisioned/joined with a Samba version older than 4.7 is upgraded to 4.7 (or later), it can happen that the garbage collection (dsdb_garbage_collect_tombstones()), triggered periodically by the 'kcc' task of 'samba' or my 'samba-tool domain tombstones expunge' corrupt the linked attributes. This is similar to Bug #13095 - Broken linked attribute handling, but it's not triggered by an originating change. The bug happens in replmd_modify_la_delete() were get_parsed_dns_trusted() generates a sorted array of struct parsed_dn based on the values in old_el->values. If the database doesn't support the sortedLinks compatibleFeatures in the @SAMBA_DSDB record, it's very likely that the array of old_dns is sorted differently than the values in old_el->values. The problem is that struct parsed_dn has just a pointer 'struct ldb_val *v' that points to the corresponding value in old_el->values. Now if vanish_links is true the damage happens here: if (vanish_links) { unsigned j = 0; for (i = 0; i < old_el->num_values; i++) { if (old_dns[i].v != NULL) { old_el->values[j] = *old_dns[i].v; j++; } } old_el->num_values = j; } old_el->values[0] = *old_dns[0].v; can change the value old_dns[1].v is pointing at! That means that some values can get lost while others are stored twice, because the LDB_FLAG_INTERNAL_DISABLE_SINGLE_VALUE_CHECK allows it to be stored. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13228 Signed-off-by: Stefan Metzmacher Reviewed-by: Ralph Boehme (cherry picked from commit a25c99c9f1fd1814c56c21848c748cd0e038eed7) --- selftest/knownfail.d/unsorted-links | 1 - source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 18 +++++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) delete mode 100644 selftest/knownfail.d/unsorted-links diff --git a/selftest/knownfail.d/unsorted-links b/selftest/knownfail.d/unsorted-links deleted file mode 100644 index 0b19ae5..0000000 --- a/selftest/knownfail.d/unsorted-links +++ /dev/null @@ -1 +0,0 @@ -^samba4.blackbox.tombstones-expunge.release-4-5-0-pre1.check_expected_unsorted_links diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 198ac84..7646f94 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -2844,11 +2844,23 @@ static int replmd_modify_la_delete(struct ldb_module *module, if (vanish_links) { unsigned j = 0; + struct ldb_val *tmp_vals = NULL; + + tmp_vals = talloc_array(tmp_ctx, struct ldb_val, + old_el->num_values); + if (tmp_vals == NULL) { + talloc_free(tmp_ctx); + return ldb_module_oom(module); + } for (i = 0; i < old_el->num_values; i++) { - if (old_dns[i].v != NULL) { - old_el->values[j] = *old_dns[i].v; - j++; + if (old_dns[i].v == NULL) { + continue; } + tmp_vals[j] = *old_dns[i].v; + j++; + } + for (i = 0; i < j; i++) { + old_el->values[i] = tmp_vals[i]; } old_el->num_values = j; } -- 1.9.1 From 1a87c5767af7f31e2e3ca34dbd2ab59f54a6a4ed Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 19 Jan 2018 11:50:55 +0100 Subject: [PATCH 3/3] dbcheck: disable fixing duplicate linked attributes until we can recover lost forward links BUG: https://bugzilla.samba.org/show_bug.cgi?id=13228 Signed-off-by: Stefan Metzmacher Reviewed-by: Ralph Boehme (cherry picked from commit 850a8027f32185e523614231cca76505134bb5e4) --- python/samba/dbchecker.py | 22 +++++++++++++++++++--- selftest/knownfail.d/dbcheck_duplicate_member | 5 +++++ 2 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 selftest/knownfail.d/dbcheck_duplicate_member diff --git a/python/samba/dbchecker.py b/python/samba/dbchecker.py index 1933740..6e4c440 100644 --- a/python/samba/dbchecker.py +++ b/python/samba/dbchecker.py @@ -708,9 +708,15 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) "Failed to fix incorrect RMD_FLAGS %u" % rmd_flags): self.report("Fixed incorrect RMD_FLAGS %u" % (rmd_flags)) - def err_orphaned_backlink(self, obj, attrname, val, link_name, target_dn): + def err_orphaned_backlink(self, obj, attrname, val, link_name, target_dn, duplicate_links): '''handle a orphaned backlink value''' self.report("ERROR: orphaned backlink attribute '%s' in %s for link %s in %s" % (attrname, obj.dn, link_name, target_dn)) + if duplicate_links: + self.report("ERROR: FATAL! Most likely the corresponding forward link got lost!") + self.report("ERROR: FATAL! See https://bugzilla.samba.org/show_bug.cgi?id=13228") + self.report("Recovery handling will be implemented in a future version") + self.report("Not removing orphaned backlink %s" % attrname) + return if not self.confirm_all('Remove orphaned backlink %s' % attrname, 'fix_all_orphaned_backlinks'): self.report("Not removing orphaned backlink %s" % attrname) return @@ -724,6 +730,11 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) def err_duplicate_links(self, obj, attrname, vals): '''handle a duplicate links value''' + self.report("ERROR: FATAL! Most likely some forward link values for attribute '%s' in '%s' got lost!" % (attrname, obj.dn)) + self.report("ERROR: FATAL! See https://bugzilla.samba.org/show_bug.cgi?id=13228") + self.report("Recovery handling will be implemented in a future version") + self.report("Not removing duplicate links in attribute '%s'" % attrname) + return if not self.confirm_all("Remove duplicate links in attribute '%s'" % attrname, 'fix_all_duplicate_links'): self.report("Not removing duplicate links in attribute '%s'" % attrname) return @@ -896,6 +907,7 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) else: reverse_syntax_oid = None + duplicate_links = False duplicate_dict = dict() duplicate_list = list() unique_dict = dict() @@ -950,6 +962,10 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) unique_dict[keystr] = dsdb_dn if len(duplicate_list) != 0: + duplicate_links = True + self.report("ERROR: FATAL! Most likely some forward link values for attribute '%s' in '%s' got lost!" % (attrname, obj.dn)) + self.report("ERROR: FATAL! See https://bugzilla.samba.org/show_bug.cgi?id=13228") + self.report("ERROR: Duplicate link values for attribute '%s' in '%s'" % (attrname, obj.dn)) for keystr in duplicate_list: d = duplicate_dict[keystr] @@ -1148,7 +1164,7 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) error_count += 1 self.err_orphaned_backlink(obj, attrname, val, reverse_link_name, - dsdb_dn.dn) + dsdb_dn.dn, duplicate_links) continue # Only warn here and let the forward link logic fix it. self.report("WARNING: Link (back) mismatch for '%s' (%d) on '%s' to '%s' (%d) on '%s'" % ( @@ -1180,7 +1196,7 @@ newSuperior: %s""" % (str(from_dn), str(to_rdn), str(to_base))) else: self.err_orphaned_backlink(res[0], reverse_link_name, obj.dn.extended_str(), attrname, - obj.dn) + obj.dn, duplicate_links) diff_count += 1 diff --git a/selftest/knownfail.d/dbcheck_duplicate_member b/selftest/knownfail.d/dbcheck_duplicate_member new file mode 100644 index 0000000..7ebb82b --- /dev/null +++ b/selftest/knownfail.d/dbcheck_duplicate_member @@ -0,0 +1,5 @@ +^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dbcheck_duplicate_member +^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.check_expected_after_duplicate_links +^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.duplicate_clean +^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dbcheck_clean2 +^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dbcheck_clean3 -- 1.9.1