h.b.furuseth@usit.uio.no wrote:
> Full_Name: Hallvard B Furuseth
> Version: HEAD
> OS: Linux
> URL:
> Submission from: (NULL) (129.240.6.233)
> Submitted by: hallvard
>
>
> RFC 3909 says both the Cancel operation and the canceled operation
> shall respond.  However I'm getting tooLate reply from Cancel
> and no response from the successfully cancelled operation:

The tooLate reply is correct; back-null doesn't ever check for cancel/abandon 
therefore the operation is *not* successfully canceled. Likewise, back-bdb/hdb 
only checks in Delete if there is a transaction retry.

The lack of reply from the original op - that's a simple oversight. 
send_ldap_ber() checks for op->o_abandon and returns silently if it's set. I 
guess we need to also check for o_cancel to let it proceed.
>
> slapd.conf:
>    include   servers/slapd/schema/core.schema
>    database  null
>    suffix   "cn=null"
> client:
>    DEL cn=null
>    sleep 0.1 sec
>    CANCEL<previous op>
>    sleep 0.2 sec
>    UNBIND
> receives the folllowing statslog:
>    conn=1 fd=9 ACCEPT from IP=127.0.0.1:48774 (IP=127.0.0.1:3890)
>    conn=1 op=0 DEL dn="cn=null"
>    conn=1 op=1 EXT oid=1.3.6.1.1.8
>    conn=1 op=1 CANCEL msg=1
>    conn=1 op=0 ABANDONED
>    conn=1 op=1 RESULT oid= err=120 text=
>    conn=1 op=2 UNBIND
>    conn=1 fd=9 closed
> and the client gets just ExtendedResponse tooLate before connection closes
>
> with the following patch to slapd:
>
> Index: delete.c
> --- delete.c	21 Jan 2009 23:40:26 -0000	1.144
> +++ delete.c	9 May 2009 22:06:07 -0000
> @@ -75,4 +75,9 @@
>   		op->o_log_prefix, op->o_req_dn.bv_val, 0, 0, 0 );
>
> +	{
> +		struct timeval timeout = { 0, 200000 };
> +		select(0, NULL, NULL, NULL,&timeout);
> +	}
> +
>   	if( op->o_req_ndn.bv_len == 0 ) {
>   		Debug( LDAP_DEBUG_ANY, "%s do_delete: root dse!\n",
> Index: result.c
> --- result.c	14 Mar 2009 05:47:43 -0000	1.329
> +++ result.c	9 May 2009 22:06:07 -0000
> @@ -418,4 +418,6 @@
>   	if ( rs->sr_err == SLAPD_ABANDON || op->o_abandon ) {
>   		rc = SLAPD_ABANDON;
> +		Statslog( LDAP_DEBUG_STATS,
> +			"%s ABANDONED\n", op->o_log_prefix, 0, 0, 0, 0 );
>   		goto clean2;
>   	}
>
>
>


-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Howard Chu writes:
> The tooLate reply is correct; back-null doesn't ever check for
> cancel/abandon therefore the operation is *not* successfully
> canceled. Likewise, back-bdb/hdb only checks in Delete if there is a
> transaction retry.

No difference if I use back-ldif, which does check for abandon.

> The lack of reply from the original op - that's a simple oversight. 
> send_ldap_ber() checks for op->o_abandon and returns silently if it's set. I 
> guess we need to also check for o_cancel to let it proceed.

The operation does not reach send_ldap_ber().  To verify, I temporarily
inserted assert(!op->o_abandon) just before send_ldap_ber()'s o_abandon
test.  send_ldap_response() notices both o_abandon and SLAPD_ABANDON,
and skips to the cleanup code in either case.

Also, backend operations that do return SLAPD_ABANDON - at least Search
in backglue and BDB - may do so without calling send_ldap_response() at
all.  Pierangelo made partial fixes in ITS#4645.  See also ITS#6059,
which I may have misunderstood somewhat.  (Sorry, Rein.)

Maybe o_abandon should only mean that:
- (a) the backend has been asked to abandon the operation,
- (b) slapd must check and react to o_cancel,
- (c) if not o_cancel, send_ldap_ber() need not send.

-- 
Hallvard

Hallvard B Furuseth wrote:
> Howard Chu writes:
>> The tooLate reply is correct; back-null doesn't ever check for
>> cancel/abandon therefore the operation is *not* successfully
>> canceled. Likewise, back-bdb/hdb only checks in Delete if there is a
>> transaction retry.
>
> No difference if I use back-ldif, which does check for abandon.
>
>> The lack of reply from the original op - that's a simple oversight.
>> send_ldap_ber() checks for op->o_abandon and returns silently if it's set. I
>> guess we need to also check for o_cancel to let it proceed.
>
> The operation does not reach send_ldap_ber().  To verify, I temporarily
> inserted assert(!op->o_abandon) just before send_ldap_ber()'s o_abandon
> test.  send_ldap_response() notices both o_abandon and SLAPD_ABANDON,
> and skips to the cleanup code in either case.
>
> Also, backend operations that do return SLAPD_ABANDON - at least Search
> in backglue and BDB - may do so without calling send_ldap_response() at
> all.  Pierangelo made partial fixes in ITS#4645.  See also ITS#6059,
> which I may have misunderstood somewhat.  (Sorry, Rein.)
>
> Maybe o_abandon should only mean that:
> - (a) the backend has been asked to abandon the operation,
> - (b) slapd must check and react to o_cancel,
> - (c) if not o_cancel, send_ldap_ber() need not send.
>
The test case with delete / back-ldif now works in HEAD. backglue and back-bdb 
should be OK now as well.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

changed notes
changed state Open to Test
moved from Incoming to Software Bugs

changed notes
changed state Test to Release

The send_ldap_response() fix is still incomplete:

When rc == LDAP_CANCELLED, the function should not send sr_matched,
sr_text, sr_ref, and sr_ctrls.  Looking at how Abandon works, it seems
slap_cleanup_play() should still see them if they are not sent?

Note, not just when the function itself sets rc = LDAP_CANCELLED,
since syncprov can set rs->sr_err = LDAP_CANCELLED too.
Which also suggests the assert( rs->sr_err == LDAP_REFERRAL ) when
sr_ref != NULL is rather dubious.  Maybe sr_ref should just
be ignored when sending something else than LDAP_REFERRAL.

Also, this:
	rc = rs->sr_err;
	if ( rc == SLAPD_ABANDON && op->o_cancel )
		rc = LDAP_CANCELLED;
will send a negative result code (SLAPD_ABANDON) if !o_cancel.
I guess it should be
	if ( rc == SLAPD_ABANDON ) {
		if ( !op->o_cancel ) goto <cleanup or clean2>;
		rc = LDAP_CANCELLED;
	}

While we're at it, I suggest to generalize this to avoid sending
result codes < 0.  Send LDAP_OTHER instead.  That's ITS#5328 again.

-- 
Hallvard

Hallvard B Furuseth wrote:
> The send_ldap_response() fix is still incomplete:
>
> When rc == LDAP_CANCELLED, the function should not send sr_matched,
> sr_text, sr_ref, and sr_ctrls.  Looking at how Abandon works, it seems
> slap_cleanup_play() should still see them if they are not sent?

Dunno what you mean.

> Note, not just when the function itself sets rc = LDAP_CANCELLED,
> since syncprov can set rs->sr_err = LDAP_CANCELLED too.
> Which also suggests the assert( rs->sr_err == LDAP_REFERRAL ) when
> sr_ref != NULL is rather dubious.  Maybe sr_ref should just
> be ignored when sending something else than LDAP_REFERRAL.

Leave the assert() in place until you have proof one way or the other.

> Also, this:
> 	rc = rs->sr_err;
> 	if ( rc == SLAPD_ABANDON&&  op->o_cancel )
> 		rc = LDAP_CANCELLED;
> will send a negative result code (SLAPD_ABANDON) if !o_cancel.

No, it won't. That is all bypassed at the top of the function.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

hyc@symas.com writes:
>Hallvard B Furuseth wrote:
>> The send_ldap_response() fix is still incomplete:

[Reversing the quoteed message]

>> Also, this:
>> 	rc = rs->sr_err;
>> 	if ( rc == SLAPD_ABANDON&&  op->o_cancel )
>> 		rc = LDAP_CANCELLED;
>> will send a negative result code (SLAPD_ABANDON) if !o_cancel.
> 
> No, it won't. That is all bypassed at the top of the function.

If so, the quoted op->o_cancel test could be skipped.  But an overlay
could set rs->sr_err = SLAPD_ABANDON during slap_response_play().

Though that ought to mean o_abandon is set, so send_ldap_ber() will drop
the message.  So you're right after all that it won't be sent.

>> Note, not just when the function itself sets rc = LDAP_CANCELLED,
>> since syncprov can set rs->sr_err = LDAP_CANCELLED too.
>> Which also suggests the assert( rs->sr_err == LDAP_REFERRAL ) when
>> sr_ref != NULL is rather dubious.  Maybe sr_ref should just
>> be ignored when sending something else than LDAP_REFERRAL.
> 
> Leave the assert() in place until you have proof one way or the other.

Haven't tested with an example of an overlay setting SLAPD_ABANDON;
could check syncprov or retcode I suppose.  Faked it with
	nanosleep(&((struct timespec){0,2E8}), 0); /*gcc or C99 code*/
	if ( op->o_abandon ) rs->sr_err = SLAPD_ABANDON;
after the op->o_callback->sc_response in slap_response_play().
Then, with this slapd.conf:
    include	servers/slapd/schema/core.schema
    referral	ldap://elsewhere/
    database	frontend
    overlay	valsort
send
    Delete "cn=urgle"
    <wait 0.1 second>
    Abandon <previous request>
lt-slapd: result.c:479: send_ldap_response:
    Assertion `rs->sr_err == 0x0a' failed.


One more detail, looking at result.c rev 1.330 send_ldap_response():
I presume the LDAP_CONNECTIONLESS branch should also ber_printf rc
rather than rs->sr_err to the response.  Not sure if Cancel makes sense
in cldap (and after V2 Bind which has no extended operation at that),
but OpenLDAP does let through Cancel after a V2 Bind.

>> When rc == LDAP_CANCELLED, the function should not send sr_matched,
>> sr_text, sr_ref, and sr_ctrls.  Looking at how Abandon works, it seems
>> slap_cleanup_play() should still see them if they are not sent?
> 
> Dunno what you mean.

I thought that was obvious, but since it isn't maybe I should ask
ldapext instead.  Cancel does seem underspecified in this regard.

Anyway, I meant: Either an operation gets cancelled/abandoned, or it
doesn't and comletes.  If these fields are set, they most likely relate
to a completed operation.  Nowhere SLAPD_ABANDON gets set, are the
others set - possibly except with back-meta/back-ldap, I haven't looked
too closely.  OTOH, code setting SLAPD_ABANDON rarely _clears_ those
fields, so I imagine they can convey information which would have been
removed (access controls maybe?)  or updated before they were sent, if
the operation had completed.

Clearing a critical control can be a bit doubtful, I'll grant.  But for
not obeying critical controls it's a choice between which error code to
return - unavailableCriticalExtension, cancelled, or something else.  Or
some controls could mark the operation as "uncancellable" (another flag
needed...)

But come to think of it, an Extended Response has some of the same
problem.  By the above reasoning, responseName and responseValue should
be dropped in a Cancelled response, as when sending protocolError to an
unrecognized reqest.  Might be a bit extreme.

-- 
Hallvard

changed notes
changed state Release to Partial

some fixes in HEAD
some fixes in RE24