Bug 13228 - Linked attribute corruption during tombstone expunge on a upgrades database from < 4.7
Summary: Linked attribute corruption during tombstone expunge on a upgrades database f...
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.7.4
Hardware: All All
: P5 critical (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
Depends on: 13095
  Show dependency treegraph
Reported: 2018-01-17 08:05 UTC by Stefan Metzmacher
Modified: 2018-02-12 11:50 UTC (History)
10 users (show)

See Also:

Simple fix for the problem using a tmp talloc array (1.33 KB, patch)
2018-01-17 08:21 UTC, Stefan Metzmacher
no flags Details
Patches for v4-7-test (part1) (20.86 KB, patch)
2018-01-25 06:53 UTC, Stefan Metzmacher
slow: review+
Patches for v4-8-test (part1) (20.86 KB, patch)
2018-01-25 06:54 UTC, Stefan Metzmacher
slow: review+
Patches for v4-8-test (part2) (73.66 KB, patch)
2018-02-06 10:45 UTC, Stefan Metzmacher
slow: review+
Patches for v4-7-test (part2) (73.66 KB, patch)
2018-02-06 10:46 UTC, Stefan Metzmacher
slow: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2018-01-17 08:05:45 UTC
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;
                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
allows it to be stored.
Comment 1 Stefan Metzmacher 2018-01-17 08:21:35 UTC
Created attachment 13914 [details]
Simple fix for the problem using a tmp talloc array

Here's a fix for the corruption bug, but I may propose a nicer fix for inclusion in master and the effected release branches.
Comment 2 Stefan Metzmacher 2018-01-17 08:24:08 UTC
As a temporary solution admins can add "server services = -kcc" to the global
section of smb.conf.
Comment 3 Ralph Böhme 2018-01-17 09:06:33 UTC
Oh, good catch! Thanks for tracking this down!
Comment 4 Tim Beale 2018-01-18 03:41:00 UTC
There is a tombstones-expunge.sh which perhaps should have detected this problem, e.g. samba4.blackbox.tombstones-expunge.release-4-5-0-pre1. It looks like it checks links were deleted but doesn't verify that the DB is in a good/correct state afterwards. (I haven't looked into the test too closely, it was just something Garming and I noticed).
Comment 5 Stefan Metzmacher 2018-01-25 06:53:56 UTC
Created attachment 13928 [details]
Patches for v4-7-test (part1)
Comment 6 Stefan Metzmacher 2018-01-25 06:54:32 UTC
Created attachment 13929 [details]
Patches for v4-8-test (part1)
Comment 7 Karolin Seeger 2018-01-25 14:03:57 UTC
Pushed to autobuild-v4-{7,8}-test.
Comment 8 Stefan Metzmacher 2018-01-26 10:03:22 UTC
Pushed to v4-{7,8}-test.

Waiting for the dbcheck improvements
Comment 9 Stefan Metzmacher 2018-02-06 10:45:48 UTC
Created attachment 13943 [details]
Patches for v4-8-test (part2)
Comment 10 Stefan Metzmacher 2018-02-06 10:46:14 UTC
Created attachment 13944 [details]
Patches for v4-7-test (part2)
Comment 11 Ralph Böhme 2018-02-06 11:01:16 UTC
Reassigning to Karolin for inclusion in 4.7 and 4.8.
Comment 12 Karolin Seeger 2018-02-06 11:07:36 UTC
(In reply to Ralph Böhme from comment #11)
Pushed to autobuild-v4-{7,8}-test.
Comment 13 Karolin Seeger 2018-02-12 11:50:05 UTC
(In reply to Karolin Seeger from comment #12)
Pushed to both branches.
Closing out bug report.