Bug 10398 - Segfault in replmd_check_urgent_objectclass due to objectClass missing on deleted object
Summary: Segfault in replmd_check_urgent_objectclass due to objectClass missing on del...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.1.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-23 12:21 UTC by Arvid Requate
Modified: 2014-09-23 09:16 UTC (History)
2 users (show)

See Also:


Attachments
gdb backtrace and object details (67.37 KB, text/x-log)
2014-01-23 12:21 UTC, Arvid Requate
no flags Details
0001-Avoid-segfault-when-objectClass-is-missing.patch (1.04 KB, patch)
2014-01-23 12:23 UTC, Arvid Requate
no flags Details
0001-Improve-objectClass-validation.patch (1.51 KB, patch)
2014-01-23 12:24 UTC, Arvid Requate
no flags Details
give an error if the objectClass is missing (1.56 KB, patch)
2014-02-27 04:08 UTC, Andrew Bartlett
no flags Details
give an error if the objectClass is missing (1.62 KB, patch)
2014-02-28 03:21 UTC, Andrew Bartlett
no flags Details
Temporary patches for v4-1-test (1.13 MB, application/octed-stream)
2014-07-08 17:21 UTC, Stefan Metzmacher
abartlet: review+
Details
Patches for v4-1-test (1.13 MB, patch)
2014-07-09 14:52 UTC, Stefan Metzmacher
metze: review+
abartlet: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate 2014-01-23 12:21:57 UTC
Created attachment 9610 [details]
gdb backtrace and object details

In two cases we have seen a segfault in replmd_check_urgent_objectclass due to the objectClass attribute missing on a deleted (\0ADEL) object.

In the second case the segfault occurred during a "samba-tool dbcheck --fix", which is a bit of a letdown ;-). Patch suggestions will be attached below.
Comment 1 Arvid Requate 2014-01-23 12:23:01 UTC
Created attachment 9611 [details]
0001-Avoid-segfault-when-objectClass-is-missing.patch
Comment 2 Arvid Requate 2014-01-23 12:24:50 UTC
Created attachment 9612 [details]
0001-Improve-objectClass-validation.patch

This adds additional checks to two other calls of replmd_check_urgent_objectclass in the same source file.
Comment 3 Andrew Bartlett 2014-02-27 04:08:11 UTC
Created attachment 9729 [details]
give an error if the objectClass is missing

The attached patch makes this a fatal error, which I think is more appropriate so we don't propagate this error further. 

We need to find where this error comes from, and stop the rot.

What do you think?
Comment 4 Andrew Bartlett 2014-02-28 03:21:02 UTC
Created attachment 9737 [details]
give an error if the objectClass is missing

(oops, I uploaded the wrong patch).
Comment 5 Arvid Requate 2014-03-12 11:56:30 UTC
Wouldn't this also stop "samba-tool dbcheck --fix" from doing it's job?
Comment 6 Arvid Requate 2014-03-12 12:01:56 UTC
Also ldbdel probably fails? What about ldbmodify?
Comment 7 Andrew Bartlett 2014-06-05 05:14:33 UTC
Just a note for others who see this to say that we have added in checks to prevent this in Samba master, by preventing inbound replication of these corrupt objects. 

Some of these patches should be backported.
Comment 8 Kamen Mazdrashki 2014-07-03 01:57:59 UTC
(In reply to comment #7)
> Just a note for others who see this to say that we have added in checks to
> prevent this in Samba master, by preventing inbound replication of these
> corrupt objects. 
> 
> Some of these patches should be backported.

And what about dbchecker.py patches?
imo those are very important too
Comment 9 Stefan Metzmacher 2014-07-08 17:21:42 UTC
Created attachment 10088 [details]
Temporary patches for v4-1-test

Once I got the master for through autobuild,
I can upload the patches with cherry-pick -x information,
but the content will be the same so this can be reviewed now.
Comment 10 Andrew Bartlett 2014-07-09 04:37:54 UTC
(In reply to comment #9)
> Created attachment 10088 [details]
> Temporary patches for v4-1-test
> 
> Once I got the master for through autobuild,
> I can upload the patches with cherry-pick -x information,
> but the content will be the same so this can be reviewed now.

What format is that patch in?  It isn't clear-text...
Comment 11 Stefan Metzmacher 2014-07-09 06:57:45 UTC
Comment on attachment 10088 [details]
Temporary patches for v4-1-test

It's bz2
Comment 12 Andrew Bartlett 2014-07-09 07:45:55 UTC
What is different in this patch set that isn't in the patches I've reviewed on the other bugs?
Comment 13 Stefan Metzmacher 2014-07-09 07:53:03 UTC
(In reply to comment #12)
> What is different in this patch set that isn't in the patches I've reviewed on
> the other bugs?

It's too large as plain text.

The top 30 patches from
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=cf33119fb9ca7f390fc2eb

This is the large one "selftest: Add tests for dbcheck detection and removal of partial objects"
34 files changed, 80719 insertions(+), 3 deletions(-)
https://git.samba.org/?p=metze/samba/wip.git;a=commit;h=d06491e77ec65255481fee70c4
Comment 14 Andrew Bartlett 2014-07-09 08:02:59 UTC
The first 3 patches duplicate bug #8449.

Otherwise, it's fine.  I do wonder what is left in master after all this, and wish we had instead the ability to quickly (eg 1month) do a 4.2 with all these changes, but given the realities doing a DRS roll-up release is the best option.  There has been a lot of good fixes in master in the past couple of months in particular.
Comment 15 Stefan Metzmacher 2014-07-09 08:05:48 UTC
(In reply to comment #14)
> The first 3 patches duplicate bug #8449.

I'll have a look.

> Otherwise, it's fine.  I do wonder what is left in master after all this, and
> wish we had instead the ability to quickly (eg 1month) do a 4.2 with all these
> changes, but given the realities doing a DRS roll-up release is the best
> option.  There has been a lot of good fixes in master in the past couple of
> months in particular.

The autolockout patches are still a difference.
Comment 16 Stefan Metzmacher 2014-07-09 08:14:17 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > The first 3 patches duplicate bug #8449.
> 
> I'll have a look.

I don't see any of the patches of #8849 ...
Comment 17 Stefan Metzmacher 2014-07-09 08:15:56 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > The first 3 patches duplicate bug #8449.
> > 
> > I'll have a look.
> 
> I don't see any of the patches of #8849 ...

The patches in the attachement are:

cf33119fb9ca7f390fc2eb70d2ee872c32d53397 CHPX s4:dsdb/kcc: use SHOW_RECYCLED instead of SHOW_DELETED in when deleting tombstone/deleted objects
92dac5636282efa09e69f8967a4ceb51eda53463 CHPX s4:dsdb/extended_dn_in: don't force DSDB_SEARCH_SHOW_RECYCLED
ad48882454dc1545948d82808e08e27b2bc7aa8d CHPX s4:dsdb/schema_load: make error message more verbose
6fa410e5e977696d7db64cec8a2bd0ba491e7480 dbcheck: Ensure dbcheck can operate with --attrs set
b0a6e1fa7fbbf4b93e148b7af79779437e051ccc kerberos: Remove un-used event context argument from smb_krb5_init_context()
37a9e1c0e33b21cb916dee73000b611cf0ea7bf3 dsdb: Specify no event context to smb_krb5_init_context() in dsdb
a7ce65c9d3fdcd6b59a397441cb9c3e5e84417e4 dsdb: Add DSDB_SEARCH_ONE_ONLY support to dsdb_module_search*()
17ed9a2d7087119250b881159eef8a6401b3867c dsdb: Do not permit nested event loops when in a transaction, use a nested event context
5322cd20a26c5280fd0c9ca5e6fb0e1e4144c1bd dsdb: Rename private_data to rootdse_private_data in rootdse
c97cae27c4dbd60d75b45d4f5613ec8e54158394 dsdb: Add more tests for DN+String and DN+Binary comparisons
d06491e77ec65255481fee70c4f9e3c674d6a4cb selftest: Add tests for dbcheck detection and removal of partial objects
994622c27aae3f17c04032ab3a85688ab5bf27af dsdb: Make it harder to corrupt the database by requiring DBCHECK or RELAX for final object deletion
db3d3131793322e3768cbc4fa3ff5c9840fba0d8 build: Exclude source4/selftest/provisions/release-4-1-0rc3 from the tarball
9ae61dd50aa2fdbe79ee70f126dd80da990e2df8 dbcheck: Directly call dn.get_rdn_{val,name}() for clarity and consistency
9995c9a6323c16d727407c946eacbe252f6df2ea dbchecker: verify and fix broken dn values
50783ac5ae44d44b5157a59ca653eb1f15aba7f7 dbchecker: make the deleted objects container detection more generic
0a962501c860302b386ebaff5437d121af15ccfd dsdb: Do not refresh the schema using the wrong event context
af2c0063177cd98851f45aa88ceefbff599c84bc dsdb: Do not store a struct ldb_dn in struct schema_data
d428a9875c396878302292d2ecc84cebc484ecc2 samba-tool dbcheck: handle missing objectClass
868389f86a34e33bc298bcc0b6f2869e0cccbfe9 dsdb: Improve missing objectClass handling
5e42c79bc2865772adec7b228722c4e158a61fac dsdb: Improve errors and checks for missing objectClass values
3f131379bb98611dcf66670489666ee165c27c84 dsdb: Clarify how the DSDB_REPL_FLAG_PRIORITISE_INCOMING flag works
cf12cc006eba99bd60deef82fff3c425048fac99 dsdb: Do not update notify_uSN until the transaction is genuinely committed to the DB
16b91e7610997b9756731e15cbf81852d8b405f9 dsdb: Further assert that we always have an objectClass and an rDN
12e3496b44e83defcfa3abf9360105bcaf3eb5fd dsdb: Ensure to sort replPropertyMetaData as UNSIGNED, not SIGNED quantities
19341443bb0197ff3657bfa6e749d6f207551a30 s4:samdb: respect SEARCH_FLAG_PRESERVEONDELETE
fc311f4a65dda344ddb6cd1bbff3f7ad93494b82 s4-samldb: Do not allow deletion of objects with RID < 1000
ae61d25a6972ccc36a5811d0fc188e6f1404a62a dsdb: Use dsdb_next_callback() rather than a no-op per-module callback
088d844aed5b47f019e673c33a29f5cadc10ff0c s4-dsdb: instanceType NC_HEAD is only allowed combined with WRITE for an originating add operation
c128b8a35a9682f272308748073fb186b9487e1c s4:dsdb/repl: make use of dcerpc_binding_handle_is_connected()
Comment 18 Stefan Metzmacher 2014-07-09 14:52:27 UTC
Created attachment 10094 [details]
Patches for v4-1-test

The same as the temporary patches but with cherry-pick information
Comment 19 Andrew Bartlett 2014-07-10 03:13:45 UTC
Comment on attachment 10094 [details]
Patches for v4-1-test

I guess I was mis-reading my local tree where I did the backport for those patches,   Sorry about that.
Comment 20 Karolin Seeger 2014-07-13 18:08:34 UTC
(In reply to comment #18)
> Created attachment 10094 [details]
> Patches for v4-1-test
> 
> The same as the temporary patches but with cherry-pick information

Pushed to autobuild-v4-1-test.
Comment 21 Karolin Seeger 2014-07-17 18:29:18 UTC
(In reply to comment #20)
> (In reply to comment #18)
> > Created attachment 10094 [details] [details]
> > Patches for v4-1-test
> > 
> > The same as the temporary patches but with cherry-pick information
> 
> Pushed to autobuild-v4-1-test.

Pushed to v4-1-test.
Closing out bug report.

Thanks!