Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: Submission from: (NULL) (193.157.198.89) Submitted by: hallvard ldap_chain_response() can send sr_text/sr_matched which refers to a different error than sr_err and rc. It saves rs-> sr_err, sr_text, sr_matched to three local variables and can restore them later, but not maintain the text and matched variables in parallel with err. Also it does not track/reset/obey REP_MATCHED_MUSTBEFREED along with sr_matched. Fails test032-chain with the asserts below. However there may be non-success result with the wrong text/matched too, that cannot be assert()ed. Come to think of it, maybe the last issues applies to ITS#6774 too? REP_MATCHED_MUSTBEFREED, mismatch between failure code and text/matched. Index: servers/slapd/back-ldap/chain.c @@ -1019,4 +1019,14 @@ ldap_chain_response( Operation *op, SlapReply *rs ) rs->sr_ref = NULL; + const char *bad_incoming_matched = NULL, *bad_incoming_text = NULL; + switch ( sr_err ) { + case LDAP_SUCCESS: + case LDAP_COMPARE_TRUE: + case LDAP_COMPARE_FALSE: + bad_incoming_matched = matched; + case LDAP_REFERRAL: + bad_incoming_text = text; + } + /* we need this to know if back-ldap returned any result */ lb.lb_lc = lc; @@ -1169,4 +1179,15 @@ cannot_chain:; dont_chain:; + switch ( sr_err ) { + case LDAP_SUCCESS: + case LDAP_COMPARE_TRUE: + case LDAP_COMPARE_FALSE: + assert( !matched || bad_incoming_matched ); + case LDAP_REFERRAL: + assert( !text || bad_incoming_text ); + } + assert( rc != LDAP_SUCCESS || + (( !text || bad_incoming_text) && + ( !matched || bad_incoming_matched ))); rs->sr_err = sr_err; rs->sr_type = sr_type;
> Full_Name: Hallvard B Furuseth > Version: HEAD > OS: > URL: > Submission from: (NULL) (193.157.198.89) > Submitted by: hallvard > > > ldap_chain_response() can send sr_text/sr_matched which refers to a > different error than sr_err and rc. > > It saves rs-> sr_err, sr_text, sr_matched to three local variables and > can restore them later, but not maintain the text and matched variables > in parallel with err. Also it does not track/reset/obey > REP_MATCHED_MUSTBEFREED along with sr_matched. > > Fails test032-chain with the asserts below. However there may be > non-success result with the wrong text/matched too, that cannot be > assert()ed. > > Come to think of it, maybe the last issues applies to ITS#6774 too? > REP_MATCHED_MUSTBEFREED, mismatch between failure code and text/matched. > > Index: servers/slapd/back-ldap/chain.c > @@ -1019,4 +1019,14 @@ ldap_chain_response( Operation *op, SlapReply *rs ) > rs->sr_ref = NULL; > > + const char *bad_incoming_matched = NULL, *bad_incoming_text = NULL; > + switch ( sr_err ) { > + case LDAP_SUCCESS: > + case LDAP_COMPARE_TRUE: > + case LDAP_COMPARE_FALSE: > + bad_incoming_matched = matched; > + case LDAP_REFERRAL: > + bad_incoming_text = text; > + } > + > /* we need this to know if back-ldap returned any result */ > lb.lb_lc = lc; > @@ -1169,4 +1179,15 @@ cannot_chain:; > > dont_chain:; > + switch ( sr_err ) { > + case LDAP_SUCCESS: > + case LDAP_COMPARE_TRUE: > + case LDAP_COMPARE_FALSE: > + assert( !matched || bad_incoming_matched ); > + case LDAP_REFERRAL: > + assert( !text || bad_incoming_text ); > + } > + assert( rc != LDAP_SUCCESS || > + (( !text || bad_incoming_text) && > + ( !matched || bad_incoming_matched ))); > rs->sr_err = sr_err; > rs->sr_type = sr_type; What do you suggest? To use a local SlapReply instead of hijacking the one passed as argument? p.
masarati@aero.polimi.it writes: > What do you suggest? To use a local SlapReply instead of hijacking the > one passed as argument? Not my call, I haven't looked enough at the code to know what it's doing. -- Hallvard
moved from Incoming to Software Bugs
Probably fixed by commit 0c8afb036a8137ba07d6a6f9172022b34aa88633 Author: Ondřej Kuzník <ondra@mistotebe.net> Date: Tue Mar 9 17:31:01 2021 +0000 ITS#9444 Manage sr_ref/sr_matched flags accordingly send_ldap_response() clears them immediately even if we never attached the data to be freed, so when we reinstate them, the flags are gone and the next send_ldap_response() doesn't consider freeing them.
Not fully fixed, needs additional work