[PATCH] Fix ldb_comparison_binary for blobs of differing lengths

[Andrew Bartlett]

On Tue, 2016-01-12 at 12:26 -0800, Jeremy Allison wrote:
> On Tue, Jan 12, 2016 at 05:24:37PM +1300, Garming Sam wrote:
> > The tests currently fail at samba4.nbt.winsreplication, which looks
> > to be due to a hard-coded ordering. Will investigate further.
> > 
> > 
> > Cheers,
> > 
> > Garming
> > 
> > On 12/01/16 16:25, Garming Sam wrote:
> > > Hi,
> > > 
> > > Recently Douglas and I have been investigating sorting issues and
> > > come across an issue with ldb_comparison_binary. In the case of
> > > two ldb_vals of the same length, the function produced the
> > > correct
> > > functionality. However, in the case of differing lengths, the
> > > function with simply return the shorter of the two (disregarding
> > > the contents completely).
> > > 
> > > In terms of how the error came to be, the function appears to be
> > > based on another which was intended to return either a YES or NO
> > > answer on whether or not two values were the same. The problem
> > > being that the function was then used as a standard comparison
> > > function for sorting.
> > > 
> > > Attached is the patch to fix the issue. As far as consequences of
> > > the error go, I'm not entirely sure what it actually broke at
> > > this
> > > point. Besides the obvious sort module, it was a default/fallback
> > > sort method and used in a few other places. A full make test is
> > > currently running to see if it breaks autobuild.
> 
> Good catch Garming, let me know when you've fixed
> the test breakage and I'll review this !

I've looked at this approach, and while I agree it is the correct way
to handle this, it does change the sort order, and therefore the
behaviour of < and > operators in ldb searches.

Now, we may decide that anybody using < and > against 'binary'
attributes is mad, except that we see here this exact thing happening
in the nbt tests as these integers happen to sort 'correctly' when
compared first by length.

Another approach is to have Samba override the 'binary' comparison for
sam.ldb only.  It is more work, but won't change existing behaviour for
other DB users.   (This isn't just ideas, I've written up code for this
option). 

What do you think?

Andrew Bartlett

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