[PATCH][PYTHON] Fix replPropertyMetaData sort order for schema extension attributes

[Andrew Bartlett]

On Wed, 2015-08-05 at 09:23 +0200, Stefan Metzmacher wrote:
> Hi Andrew,
> 
> >>>> - Do we have a test that checks the objectClass sorting against the 
> >>>> LDAP
> >>>> server
> >>>>   (and passed against windows)?
> >>>
> >>> No, not currently.  We should have it check that the correct
> >>> attributeID is added, that it is sorted, and that when entries are
> >>> modified, replaced or deleted, that the version is incremented. 
> >>
> >> That an answer to the wrong question...
> > 
> > Ah. :-)
> > 
> >> My question was about the sorting of objectClass values:
> >> https://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=c74751d5e89e9b36a0987589f4dd0e481137561d
> > 
> > I have no intention of matching Windows behaviour there.  The docs (and
> > I should reference those) say the order is undefined for AUX classes.
> 
> I'm sorry but we really need to match Windows, everything else will
> cause problems
> with poorly written admin scripts.

> And we need tests to prove it our behavior matches a Windows server.

Then please NAK the patch until you can come up with a behaviour you can
live with. 

To be clear, each time the objectClass list is modified, the portion of
the list with AUX classes reverses. 

> > What windows actually does is store them in the inverse of the order you
> > set them in :-)
> 
> We need tests to prove that, it's also possible that they are sorted by
> governsID.

No, I looked into it extensively, using a number of different
objectClass values, with different OIDs and prefixes (and so attributeID
values).  

> > (See the code in our DRS to LDB handler for objectClass that inverts the
> > order). 
> > 
> > What this code does is make the input order deterministic, so that the
> > rest of the calculation is deterministic. 
> 
> I think Windows is also deterministic, given specific input values.

No, it isn't.  Please actually do ldbedit against a Windows server and
see for yourself.

> >> Can you initialize alpha_sorted to NULL in the declaration we had a
> >> number of security
> >> bugs related to unitialized pointers already...
> >> The code would also look much clearer if we would have:
> >>
> >> struct class_list *unsorted = NULL;
> >> struct class_list *sorted = NULL;
> >> ...
> >> struct class_list *alpha_sorted = NULL;
> >>
> >> Instead of the spaghetti:
> >>
> >> struct class_list *unsorted = NULL, *sorted = NULL, *current = NULL,
> >>         *poss_parent = NULL, *new_parent = NULL,
> >>         *current_lowest = NULL, *current_lowest_struct = NULL,
> >>         *alpha_sorted;
> > 
> > Certainly!
> > 
> >>>> - Please add BUG: https:... tags to the commit messages
> >>>>   We want to backport this at least to v4-3-test and v4-2-test, but
> >>>> maybe also to v4-1-test...
> >>>
> >>> The key is to backport 61b978872fe86906611f64430b2608f5e7ea7ad8, then
> >>> these to fix up broken databases. 
> >>>
> >>> The big remaining issue is that our msDS-IntID handling is totally
> >>> broken.  We use that only when pushing the values over the network with
> >>> GetNCChanges, but not during storage in replPropertyMetaData.  This
> >>> means for the an object with the same attribute modified on two servers
> >>> we can end up with metadata for an entry twice, once for each style of
> >>> attributeID.
> >>
> >> Ok, do we have bug reports for all this problems?
> > 
> > Not yet.
> 
> Please add them and cc me.

Certainly.

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba