[Issue 10060] New: libldap: ldap_result return value differs depending on if a search response is already in the response list or not
https://bugs.openldap.org/show_bug.cgi?id=10060
Issue ID: 10060 Summary: libldap: ldap_result return value differs depending on if a search response is already in the response list or not Product: OpenLDAP Version: unspecified Hardware: All OS: All Status: UNCONFIRMED Keywords: needs_review Severity: normal Priority: --- Component: libraries Assignee: bugs@openldap.org Reporter: david@hardeman.nu Target Milestone: ---
ldap_result(3) is defined here: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
On this line, ldap_result will call wait4msg: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
wait4msg is defined here: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
If a response for a given msgid is already stored in the response list, and all=1, the whole message chain will be returned by the call from wait4msg to chkResponseList here: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
And rc will be set on the next line to the head of the message chain.
If a response for a given msgid is *not* already stored in the response list, wait4msg will eventually call try_read1msg here: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
If try_read1msg manages to receive the final search result message for a given msgid, it will be noted here: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
The whole chain will be returned here: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
And the return code will be the tag of the *last* message in the chain, as opposed to the *first* message in the chain: https://git.openldap.org/openldap/openldap/-/blob/master/libraries/libldap/r...
Which will result in different return codes for chains of multiple messages (i.e. search results with all=1).
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #1 from David Härdeman david@hardeman.nu --- Created attachment 969 --> https://bugs.openldap.org/attachment.cgi?id=969&action=edit Test case
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #2 from David Härdeman david@hardeman.nu --- The output from the test case is: ldap_search_ext = 0, msgid = 2 ldap_search_ext = 0, msgid = 3 ldap_result rc = 101, msgid = 3, result is now 0x0x55b2ddc53990 Message: ID 3, type 100 Message: ID 3, type 100 Message: ID 3, type 100 Message: ID 3, type 101 ldap_result rc = 100, msgid = 2, result is now 0x0x55b2ddc538f0 Message: ID 2, type 100 Message: ID 2, type 100 Message: ID 2, type 100 Message: ID 2, type 101
Note the difference in "ldap_result rc" (101 vs 100, it should be 101 in both cases)
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #3 from Ondřej Kuzník ondra@mistotebe.net --- Hi David, thanks for the report.
You are correct, while the ldap_result manpage doesn't specify whether either behaviour is correct, the C LDAP API draft[0] is quite explicit about the result being "the type of the first result returned in the res parameter."
On the other hand, ldapsearch depends on the current behaviour so it might need updating in lockstep.
[0]. https://datatracker.ietf.org/doc/html/draft-ietf-ldapext-ldap-c-api-05#secti...
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #4 from Ondřej Kuzník ondra@mistotebe.net ---
ldap_result rc" (101 vs 100, it should be 101 in both cases)
BTW this should be LDAP_RES_SEARCH_ENTRY (100) in both cases as outlined in the draft - as that's the first message in result.
https://bugs.openldap.org/show_bug.cgi?id=10060
Ondřej Kuzník ondra@mistotebe.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Ever confirmed|0 |1 Assignee|bugs@openldap.org |ondra@mistotebe.net Status|UNCONFIRMED |IN_PROGRESS
--- Comment #5 from Ondřej Kuzník ondra@mistotebe.net --- https://git.openldap.org/openldap/openldap/-/merge_requests/627
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #6 from David Härdeman david@hardeman.nu --- Changing it to LDAP_RES_SEARCH_ENTRY (100) in both cases will (probably) cause more breakage though.
For example, python-ldap CI tests will break...as might third party software which uses python-ldap.
Not saying it's wrong to change to LDAP_RES_SEARCH_ENTRY (the C LDAP API draft you cited seems pretty clear), but I just wanted to point out that more downstream software will have to be changed...
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #7 from Howard Chu hyc@openldap.org --- I'm inclined to make this a WONTFIX. Changing a 20+ year old library behavior just seems like a bad idea at this point. (Note that this in particular was also one of the behaviors that was explicitly changed between UMich LDAP and OpenLDAP 1.0. Bugs here go back a long way and any changes will have far-reaching effects.)
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #8 from Ondřej Kuzník ondra@mistotebe.net --- On Wed, May 31, 2023 at 08:05:58PM +0000, openldap-its@openldap.org wrote:
I'm inclined to make this a WONTFIX. Changing a 20+ year old library behavior just seems like a bad idea at this point. (Note that this in particular was also one of the behaviors that was explicitly changed between UMich LDAP and OpenLDAP 1.0. Bugs here go back a long way and any changes will have far-reaching effects.)
That's also an option if we don't want to pull it in 2.7. In that case I could put something like this in the BUGS section of the manpage:
---- 8< ----
BUGS
When there are multiple messages in **result including a result message (common with searches), the return value of ldap_result(, MSG_ALL,) will vary depending on various factors. Some implementations have started relying on this behaviour and as such it is unlikely to be be fixed in the 2.x release series. To get the message type you're looking for, use ldap_first_message() or ldap_parse_result() as needed.
---- 8< ----
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #9 from David Härdeman david@hardeman.nu --- Actually, I think changing both cases to return 101 might make more sense.
As shown by Ondřej's PR, ldapsearch already expects that to be the return code (and has done so for a long time).
Further, python-ldap expects 101 to be the return code as well (as do projects using python-ldap, implicitly).
While I understand the appeal of not changing the status quo, not fixing this runs the risk of difficult to reproduce bugs, especially in threaded environments where the return code might differ depending on network delays and the order in which different operations complete (and are processed).
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #10 from Howard Chu hyc@openldap.org --- Btw, 25 years - this is the commit I was thinking of fa2da63ca4d3f48793de5421f56c23c1c6d794ab
https://git.openldap.org/openldap/openldap/-/commit/fa2da63ca4d3f48793de5421...
https://bugs.openldap.org/show_bug.cgi?id=10060
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords|needs_review | Target Milestone|--- |2.7.0
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #11 from Ondřej Kuzník ondra@mistotebe.net --- The new MR now does the opposite, could you test on your end (python-ldap test suite seems to work for me but confirmation welcome). https://git.openldap.org/openldap/openldap/-/merge_requests/631
https://bugs.openldap.org/show_bug.cgi?id=10060
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|IN_PROGRESS |RESOLVED Resolution|--- |TEST
--- Comment #12 from Quanah Gibson-Mount quanah@openldap.org --- • 3676f3ad by Ondřej Kuzník at 2023-07-19T14:53:47+00:00 ITS#10060 Return tag of last message if all=LDAP_MSG_ALL
• 4b7b2172 by Ondřej Kuzník at 2023-07-19T14:53:47+00:00 ITS#10060 Try harder to find a finished operation with msgid=LDAP_RES_ANY
https://bugs.openldap.org/show_bug.cgi?id=10060
--- Comment #13 from Howard Chu hyc@openldap.org --- Fwiw, it makes more sense to return the tag of the last message. The caller can easily obtain the tag of the first message, at nearly zero cost. But as the message chain can be arbitrarily long, it would take more effort for the caller to obtain the tag of the last message if we didn't return it.
-
openldap-its＠openldap.org