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.