Re: (ITS#4860) Sets' enhancement

[jclarke@linagora.com]

This is a multi-part message in MIME format.
--------------080208090603090201080004
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit

Pierangelo Masarati a écrit :
>> The patch attached corrects this problem on RE23 and HEAD for me
>> and doesn't have any side effects on our test set. However, it
>> may not be the "right" way - please correct if necessary!
>
> It seems to work correctly, but there seems to be an easier fix: just
> NULL out the pointer to lset/rset, respectively.  I'm patching the
> code this way, please test if you get a chance.

Whoops - looks like I spoke too soon. After more testing, I've
realized that the same commit this patch applied to actually changed the
behaviour of the "+" operation in the slap_set_join() function when
operating on empty sets, and can cause errors.

The attached proposed patch restores the original design. The difference
is minor, but can cause slapd to hang indefinitely on some sets, like
the one in the generic example below.

I understand an element in a set can have 3 possible states:
1) non-empty string (example: "cn=user,ou=people,dc=example,dc=com")
2) empty string (example: "", BER_BVISEMPTY)
3) NULL (BER_BVISNULL)

Below I represent sets in {element1, element2} notation, to describe the
current behaviour (in HEAD) for slap_set_join on the '+' operation:
- {"One", "Two"} + {"Three", "Four"} ==> {"OneThree", "OneFour",
"TwoThree", "TwoFour"}
- {"One", "Two"} + {""} ==> {"One", "Two"}
- {"One", "Two"} + {NULL} ==> {"One", "Two"}

Consider the following set:
set="[ldap:///] + [ldap:///<searchbase>??<scope>?<filter1>] +
[??<scope>?<filter2>]"

Assuming [ldap:///<searchbase>??<scope>?<filter1>] returns a DN, this
works. However, if [ldap:///<searchbase>??<scope>?<filter1>] returns no
entries, this value is NULL. Therefore, the rest of the set becomes:
"[ldap:///] + {NULL} + [??<scope>?<filter2>]"
...which is handed to the '+' operation in slap_set_join, to become:
[ldap:///??<scope>?<filter2>]
...which obviously isn't valid.

The attached patch changes the last case of the current behaviour for
slap_set_join on the '+' operation. This was already the behaviour
before version 1.24.2.6 Thu Sep 13 20:43:29 2007 UTC:
- {"One", "Two"} + {NULL} ==> {NULL}

Thus, in the above example, the whole set would resolve to NULL, and
access is not granted.

Sorry for the confusion. Can this patch be integrated?

Regards,
Jonathan

-- 
Jonathan Clarke

Cellule OSSA - Groupe LINAGORA
27 rue de Berri, 75008 Paris
Tél: 01 58 18 68 28, fax: 01 58 18 68 29
http://www.linagora.com - http://www.08000linux.com

--------------080208090603090201080004
Content-Type: text/x-patch;
 name="jonathan-clarke-20071109.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="jonathan-clarke-20071109.patch"

--- servers/slapd/sets.c.orig	2007-11-01 16:47:35.000000000 +0100
+++ servers/slapd/sets.c	2007-11-01 16:48:21.000000000 +0100
@@ -277,25 +277,13 @@
 		j = slap_set_size( lset );
 
 		/* handle empty set cases */
-		if ( i == 0 ) {
-			if ( j == 0 ) {
-				set = cp->set_op->o_tmpcalloc( i * j + 1, sizeof( struct berval ),
-						cp->set_op->o_tmpmemctx );
-				if ( set == NULL ) {
-					break;
-				}
-				BER_BVZERO( &set[ 0 ] );
-				break;
-
-			} else {
-				set = set_dup( cp, lset, SLAP_SET_LREF2REF( op_flags ) );
-				lset = NULL;
+		if ( i == 0 || j == 0 ) {
+			set = cp->set_op->o_tmpcalloc( i * j + 1, sizeof( struct berval ),
+					cp->set_op->o_tmpmemctx );
+			if ( set == NULL ) {
 				break;
 			}
-
-		} else if ( j == 0 ) {
-			set = set_dup( cp, rset, SLAP_SET_RREF2REF( op_flags ) );
-			rset = NULL;
+			BER_BVZERO( &set[ 0 ] );
 			break;
 		}
 

--------------080208090603090201080004--