Full_Name: Hallvard B Furuseth Version: HEAD OS: Linux URL: Submission from: (NULL) (129.240.6.233) Submitted by: hallvard Back-relay issues - fixing some, awaiting confirmation/discussion of others: * The handlers for Abandon, Cancel and connection-init/destroy should not exist, as far as I can tell. They forward the call to the underlying backend database, but that means it receives the call twice: Slapd calls these handlers for all databases anyway (unless one returns success in case of Cancel), since there's no DN on which to dispatch. See abandon.c:fe_op_abandon(), cancel.c:cancel_extop(), and backend.c:backend_connection_init()/backend_connection_destroy(). The Cancel handler seems broken anyway. It sets op->o_cancel = LDAP_CANNOT_CANCEL on the Cancel operation. Presumably it intended the to-be-cancelled operation. I'm "#if 0"ing these out for now in case I've missed something. Can remove the code later. * Fixing RB_ERR (set rs->sr_err) and RB_SEND (send if error) handling: relay_back_select_backend() did not obey (!(fail_mode & RB_ERR)) except when it caught recursion. LDAP_SUCCESS makes no sense combined with these flags. relay_back_operational() could set rs->sr_err = success on failure, which seemed a bit excessive. (Though it should return success.) Removing the RB_ERR flag. relay_back_chk_referrals() can send success on failure. Except the function is not used, so I'm just #if 0ing and commenting it. This also means relay_back_select_backend() without these flags could just as well get SlapReply rs == NULL, which again means rs==NULL could function as a !RB_ERR flag. Not doing that yet, in case the changes below lead to some reason not to. * Fixing relay_back_select_backend() crash when select_backend()==NULL: include servers/slapd/schema/core.schema database relay suffix cn=relay,cn=test overlay rwm rwm-suffixmassage cn=nowhere ldapsearch -b cn=urgle,cn=relay,cn=test -s base * back-relay can be configured to cause infinite recursion. This dumps core with the above search: include servers/slapd/schema/core.schema database relay subordinate suffix cn=relay,cn=test relay cn=relay,cn=test database null suffix cn=test There are some tests to catch that, but half-hearted ones. And some comments about it, like this: /* FIXME: this test only works if there are no overlays, so * it is nearly useless; if made stricter, no nested back-relays * can be instantiated... too bad. */ if ( bd == NULL || bd == op->o_bd ) { return LDAP_SUCCESS; } I don't know how that's worse in this case than in other cases, nor what exactly how that snippet is supposed to work, so I don't know if the suggestion below fixes it and the test can be removed. The 'bd == op->o_bd' part can be removed since relay_back_select_backend() now returns bd==NULL when that happens: (bd->be_private == op->o_bd->be_private) implies (bd == op->o_bd). Anyway, recursion can now be properly caught with op->o_extra. Wrap calls to the underlying backend (e.g. in relay_back_op) in - untested: OpExtraDB oex; oex.oe_db = op->o_bd; oex.oe.oe_key = op->o_bd->be_private; LDAP_SLIST_INSERT_HEAD( &op->o_extra, &oex.oe, oe_next ); ... LDAP_SLIST_REMOVE( &op->o_extra, &oex.oe, OpExtra, oe_next ); and check in relay_back_select_backend() with relay_back_info *ri = (relay_back_info *)op->o_bd->be_private; OpExtra *oex; LDAP_SLIST_FOREACH( oex, &op->o_extra, oe_next ) { if ( oex->oe_key == ri ) <caught recursion, fail and return NULL>; } However, does the comment quoted above mean that it's desirable to allow a few rounds of recursion (a back-relay instance calling itself)? If so, define a config parameter relay-max-recursion with default=0, and define a struct with the OpExtra members + a counter which says how many rounds of recursion the operation has had so far for this database. See the OpExtra comment in slap.h.
changed notes changed state Open to Active moved from Incoming to Software Bugs
Questions: * relay_back_operational() sets up callbacks. Should it? Looks harmless, but as far as I can tell, be->be_operational() functions do not use them, since they (should) send no response. * There is no relay_back_chk_controls(). Should there be? Though I think DNs would then be rewritten four times the same way for each operation:-( Already operational, has_subordinates and finally the operation itself does. And possibly for access controls. I've factored op.c code out to table-driven handlers and a macro, and cleaned away those '#if 0's. Fixed more problems: * Search referrals should have a scope. * relay_back_op_extended() was (still) broken. The handler should return a result which caller should send, so it must set sr->sr_ref without freeing it. Setting REP_REF_MUSTBEFREED instead, and droping the RB_SEND requirement in fail_mode. * For readability, fixed return values from relay_back_chk_referrals() and other unused handlers. (chk_referrals may be unfixable.) * relay_back_entry_<get/release>_rw() returned operationsError for failure. Failing with noSuchObject/unwillingToPerform instead. * relay_back_entry_release_rw() leaked entries when bd->be_release==0. For paranoia, fixed it only when the entry's e_private == NULL. > * The handlers for Abandon, Cancel and connection-init/destroy > should not exist, as far as I can tell. They forward the call to the > underlying backend database, but that means it receives the call twice: So does Unbind. Removed the handler. > * back-relay can be configured to cause infinite recursion. (...) > Anyway, recursion can now be properly caught with op->o_extra. (...) Needed a unique key per <operation type, relay database> combination. Otherwise things like backend_group() called from another operation failed when looking up a relayed DN via relay_back_entry_get_rw(). That fixed relay_back_operational() and relay_back_has_subordinates(), or at least I assume that's what their FIXMEs were about. -- Hallvard
changed notes changed state Active to Open
changed notes changed state Open to Partial
Some fixes in HEAD Some fixes in RE24