Issue 8866 - RFE: slapo-constraint to return filter used in diagnostic message
Summary: RFE: slapo-constraint to return filter used in diagnostic message
Status: VERIFIED FIXED
Alias: None
Product: OpenLDAP
Classification: Unclassified
Component: overlays (show other issues)
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: 2.5.0
Assignee: OpenLDAP project
URL:
Keywords:
: 7738 (view as issue list)
Depends on:
Blocks:
 
Reported: 2018-06-20 09:56 UTC by Michael Ströder
Modified: 2020-10-14 21:16 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Michael Ströder 2018-06-20 09:56:08 UTC
Full_Name: 
Version: 
OS: 
URL: 
Submission from: (NULL) (213.240.182.62)


See motivation and disclosure considerations in list archive:

https://www.openldap.org/lists/openldap-devel/201711/msg00003.html

Patch will follow.
Comment 1 Michael Ströder 2018-06-20 11:25:57 UTC
Rationale:
This patch is meant to enhance user experience in case a client software 
is used to maintain data directly via LDAP. This is a real-world issue.

Find the patch against master here:
https://www.stroeder.com/temp/0001-ITS-8866-slapo-unique-to-return-filter-used-in-diagn.patch

Also cleanly applies to RE24 and therefore
could be easily added to upcoming release 2.4.47. ;-)

Comment 2 Michael Ströder 2018-06-20 11:41:52 UTC
On 06/20/2018 01:26 PM, michael@stroeder.com wrote:
> Find the patch against master here:
> https://www.stroeder.com/temp/0001-ITS-8866-slapo-unique-to-return-filter-used-in-diagn.patch

Ouch! This was not yet complete. I'll come up with a new revision soon.

Ciao, Michael.

Comment 3 Michael Ströder 2018-06-20 13:06:42 UTC
On 06/20/2018 01:41 PM, Michael Ströder wrote:
> Ouch! This was not yet complete. I'll come up with a new revision soon.

Please review this patch:
https://www.stroeder.com/temp/0001-ITS-8866-slapo-unique-to-return-filter-used-in-diagn.patch

Disclaimer: I'm not a C programmer. Thanks for your patience.

Ciao, Michael.

Comment 4 Michael Ströder 2018-07-04 10:16:19 UTC
On 06/20/2018 01:25 PM, Michael Ströder wrote:
> This patch is meant to enhance user experience in case a client software 
> is used to maintain data directly via LDAP. This is a real-world issue.
> 
> Find the patch against master here:
> https://www.stroeder.com/temp/0001-ITS-8866-slapo-unique-to-return-filter-used-in-diagn.patch 
> 
> Also cleanly applies to RE24 and therefore
> could be easily added to upcoming release 2.4.47. ;-)

Any chance to see this in 2.4.47?

It simply works and the patch was also reviewed by another C programmer.

Ciao, Michael.

Comment 5 Michael Ströder 2018-07-30 17:56:05 UTC
see also ITS#7738

Comment 6 Michael Ströder 2018-07-30 17:58:47 UTC
Can someone correct the subject line of the ticket?

Should of course mention slapo-unique instead of slapo-constraint.

Comment 7 Quanah Gibson-Mount 2018-10-26 01:59:42 UTC
changed state Open to Test
moved from Incoming to Software Enhancements
Comment 8 OpenLDAP project 2018-10-26 01:59:52 UTC
applied to master
Comment 9 Quanah Gibson-Mount 2018-10-26 01:59:52 UTC
changed notes
Comment 10 Quanah Gibson-Mount 2018-10-26 02:01:12 UTC
--On Monday, July 30, 2018 6:58 PM +0000 michael@stroeder.com wrote:

> Can someone correct the subject line of the ticket?
>
> Should of course mention slapo-unique instead of slapo-constraint.

This patch has been applied to OpenLDAP HEAD with a correction to the 
memory allocation functions.  I'll discuss with Howard about whether or not 
to add it to 2.4.47.

--Quanah

--

Quanah Gibson-Mount
Product Architect
Symas Corporation
Packaged, certified, and supported LDAP solutions powered by OpenLDAP:
<http://www.symas.com>


Comment 11 Michael Ströder 2018-10-26 08:12:05 UTC
On 10/26/18 4:01 AM, Quanah Gibson-Mount wrote:
> --On Monday, July 30, 2018 6:58 PM +0000 michael@stroeder.com wrote:
> 
>> Can someone correct the subject line of the ticket?
>>
>> Should of course mention slapo-unique instead of slapo-constraint.
> 
> This patch has been applied to OpenLDAP HEAD with a correction to the
> memory allocation functions.  I'll discuss with Howard about whether or
> not to add it to 2.4.47.

With your memory allocation correction, op->o_tmpalloc() and
op->o_tmpfree(), this does not work as expected with 2.4.x. It does
*not* return the filter used for uniqueness check.
(I did not test master yet.)

Example for expected diagnostic message (with my original patch applied
to 2.4.46):

   non-unique attributes found with (|(gidNumber=30041))

Result with patch backported from git master using op->o_tmpalloc() and
op->o_tmpfree():

   non-unique attributes found with non-unique attribute
                                    ^^^^^^^^^^^^^^^^^^^^
                        Seems to repeat parts of the buffer?

In my original patch the use of ch_malloc() and ch_free() was simply
copied from other overlay code. There are many occurences of ch_malloc()
and ch_free() throughout the whole code.

Does op->o_tmpalloc() and op->o_tmpfree() work correctly in RE24 branch?

Ciao, Michael.

Comment 12 Ondřej Kuzník 2018-10-26 14:21:35 UTC
On Fri, Oct 26, 2018 at 08:12:13AM +0000, michael@stroeder.com wrote:
> On 10/26/18 4:01 AM, Quanah Gibson-Mount wrote:
>> This patch has been applied to OpenLDAP HEAD with a correction to the
>> memory allocation functions.  I'll discuss with Howard about whether or
>> not to add it to 2.4.47.
> 
> With your memory allocation correction, op->o_tmpalloc() and
> op->o_tmpfree(), this does not work as expected with 2.4.x. It does
> *not* return the filter used for uniqueness check.
> (I did not test master yet.)
> 
> [...]
> Result with patch backported from git master using op->o_tmpalloc() and
> op->o_tmpfree():
> 
>    non-unique attributes found with non-unique attribute
>                                     ^^^^^^^^^^^^^^^^^^^^
>                         Seems to repeat parts of the buffer?
>
> In my original patch the use of ch_malloc() and ch_free() was simply
> copied from other overlay code. There are many occurences of ch_malloc()
> and ch_free() throughout the whole code.

Yes, but `key` had already been freed a few lines earlier and using
o_tmpalloc reliably exposes the issue where ch_malloc just masks it.
This is now fixed in master.

-- 
Ondřej Kuzník
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

Comment 13 Michael Ströder 2018-10-26 15:04:48 UTC
On 10/26/18 4:21 PM, Ondřej Kuzník wrote:
> Yes, but `key` had already been freed a few lines earlier and using
> o_tmpalloc reliably exposes the issue where ch_malloc just masks it.

Ouch!

> This is now fixed in master.

Thanks. Everything now works like a charm also with RE24.

Ciao, Michael.

Comment 14 Ondřej Kuzník 2020-04-02 10:18:29 UTC
*** Issue 7738 has been marked as a duplicate of this issue. ***