OpenLDAP
Up to top level
Build   Contrib   Development   Documentation   Historical   Incoming   Software Bugs   Software Enhancements   Web  

Logged in as guest

Viewing Software Bugs/5540
Full headers

From: unix.gurus@gmail.com
Subject: Normalization assertion in attr.c
Compose comment
Download message
State:
0 replies:
24 followups: 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24

Major security issue: yes  no

Notes:

Notification:


Date: Fri, 30 May 2008 00:01:04 GMT
From: unix.gurus@gmail.com
To: openldap-its@OpenLDAP.org
Subject: Normalization assertion in attr.c
Full_Name: Sean Burford
Version: 2.4.9
OS: Linux x86_64
URL: ftp://ftp.openldap.org/incoming/sean-burford-080529.tar.gz.1
Submission from: (NULL) (76.104.224.20)


Adding equality and substring matching to an existing in use attribute causes
the schema and database contents to mismatch.  I added equality and substring
matching to an attribute in my schema.  A modification of an attribute value was
propogated a few days later, triggering the assertion and taking my replicas
offline.  Each restart replicated the change and triggered the assertion again.

When no matching is specified, new database entries do not have their normalized
value populated in the database.  When the matching is added, new database
entries with have their normalized value populated in the database.  There is an
assertion in attr.c that checks that attributes from the database have the
expected normalization.

I have attached a script and config to demonstrate the issue
(ftp://ftp.openldap.org/incoming/sean-burford-080529.tar.gz.1)

These files are in the tarball:

slapd.d/ contains a server configuration that is suitable for reproducing the
problem.

script/trigger-assertion.sh performs the ldap operations necessary to trigger
the assertion.  The important ones are:
 - An attribute is added to the schema without equality matching.
 - An entry is added to the directory that uses the new attribute.
 - The schema is modified so that the attribute has equality matching.
 - Another attribute value is added to the entry, triggering the assertion.

patch/attr-assertion.patch causes the operation to be rejected and logged,
rather than triggering the assertion.  It might not be the optimal patch
for this problem, but it prevents the assertion and rejects the operation.


Followup 1

Download message
Date: Thu, 29 May 2008 17:20:29 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
------=_Part_4694_18767492.1212106829556
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: base64
Content-Disposition: inline

VGhpcyBiYWNrdHJhY2UgbWF5IHByb3ZpZGUgc29tZSBmdXJ0aGVyIGluZm86CgojMCAgMHgwMDAw
N2Y3MjU1OGFhMDk1IGluIHJhaXNlICgpIGZyb20gL2xpYi9saWJjLnNvLjYKIzEgIDB4MDAwMDdm
NzI1NThhYmFmMCBpbiBhYm9ydCAoKSBmcm9tIC9saWIvbGliYy5zby42CiMyICAweDAwMDA3Zjcy
NTU4YTMyZGYgaW4gX19hc3NlcnRfZmFpbCAoKSBmcm9tIC9saWIvbGliYy5zby42CiMzICAweDAw
MDAwMDAwMDA0MjZlNDEgaW4gYXR0cl9tZXJnZSAoZT08dmFsdWUgb3B0aW1pemVkIG91dD4sCiAg
ICBkZXNjPTx2YWx1ZSBvcHRpbWl6ZWQgb3V0PiwgdmFscz0weDk4Y2ViMCwgbnZhbHM9MHg5NTNi
ZjApIGF0IGF0dHIuYzo0NzcKIzQgIDB4MDAwMDAwMDAwMDQ2ODFiYyBpbiBtb2RpZnlfYWRkX3Zh
bHVlcyAoZT0weDQwODVmN2IwLCBtb2Q9MHg5OGNlNzAsCiAgICBwZXJtaXNzaXZlPTAsIHRleHQ9
MHg0MDg2MGNkMCwgdGV4dGJ1Zj0weDQwODVmOGIwICIw77+9XDIzMCIsIHRleHRsZW49MjU2KQog
ICAgYXQgbW9kcy5jOjE1NQojNSAgMHgwMDAwMDAwMDAwNDhlNGJmIGluIGhkYl9tb2RpZnlfaW50
ZXJuYWwgKG9wPTB4OTUyNDAwLCB0aWQ9MHg5NTM2NDAsCiAgICBtb2RsaXN0PTB4OThjZTcwLCBl
PTB4NDA4NWY3YjAsIHRleHQ9MHg0MDg2MGNkMCwKICAgIHRleHRidWY9MHg0MDg1ZjhiMCAiMO+/
vVwyMzAiLCB0ZXh0bGVuPTI1NikgYXQgbW9kaWZ5LmM6MTAxCiM2ICAweDAwMDAwMDAwMDA0OGVl
YWQgaW4gaGRiX21vZGlmeSAob3A9MHg5NTI0MDAsIHJzPTB4NDA4NjBjYjApCiAgICBhdCBtb2Rp
ZnkuYzo1NzgKIzcgIDB4MDAwMDAwMDAwMDQzNDZlZCBpbiBmZV9vcF9tb2RpZnkgKG9wPTB4OTUy
NDAwLCBycz0weDQwODYwY2IwKQogICAgYXQgbW9kaWZ5LmM6MzAwCiM4ICAweDAwMDAwMDAwMDA0
MzRmZGEgaW4gZG9fbW9kaWZ5IChvcD0weDk1MjQwMCwgcnM9MHg0MDg2MGNiMCkKICAgIGF0IG1v
ZGlmeS5jOjE3NQojOSAgMHgwMDAwMDAwMDAwNDFlMTkzIGluIGNvbm5lY3Rpb25fb3BlcmF0aW9u
IChjdHg9MHg0MDg2MGUwMCwKICAgIGFyZ192PTx2YWx1ZSBvcHRpbWl6ZWQgb3V0PikgYXQgY29u
bmVjdGlvbi5jOjEwODQKIzEwIDB4MDAwMDAwMDAwMDQxZTk5NSBpbiBjb25uZWN0aW9uX3JlYWRf
dGhyZWFkIChjdHg9MHg0MDg2MGUwMCwKICAgIGFyZ3Y9PHZhbHVlIG9wdGltaXplZCBvdXQ+KSBh
dCBjb25uZWN0aW9uLmM6MTIxMQojMTEgMHgwMDAwMDAwMDAwNTQzMGRhIGluIGxkYXBfaW50X3Ro
cmVhZF9wb29sX3dyYXBwZXIgKHhwb29sPTB4OGQ3YzYwKQogICAgYXQgdHBvb2wuYzo2NjMKIzEy
IDB4MDAwMDdmNzI1NmU2ZDNmNyBpbiBzdGFydF90aHJlYWQgKCkgZnJvbSAvbGliL2xpYnB0aHJl
YWQuc28uMAojMTMgMHgwMDAwN2Y3MjU1OTRmYjJkIGluIGNsb25lICgpIGZyb20gL2xpYi9saWJj
LnNvLjYKIzE0IDB4MDAwMDAwMDAwMDAwMDAwMCBpbiA/PyAoKQo=
------=_Part_4694_18767492.1212106829556
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: base64
Content-Disposition: inline

VGhpcyBiYWNrdHJhY2UgbWF5IHByb3ZpZGUgc29tZSBmdXJ0aGVyIGluZm86PGJyPjxicj4jMCZu
YnNwOyAweDAwMDA3ZjcyNTU4YWEwOTUgaW4gcmFpc2UgKCkgZnJvbSAvbGliL2xpYmMuc28uNjxi
cj4jMSZuYnNwOyAweDAwMDA3ZjcyNTU4YWJhZjAgaW4gYWJvcnQgKCkgZnJvbSAvbGliL2xpYmMu
c28uNjxicj4jMiZuYnNwOyAweDAwMDA3ZjcyNTU4YTMyZGYgaW4gX19hc3NlcnRfZmFpbCAoKSBm
cm9tIC9saWIvbGliYy5zby42PGJyPgojMyZuYnNwOyAweDAwMDAwMDAwMDA0MjZlNDEgaW4gYXR0
cl9tZXJnZSAoZT0mbHQ7dmFsdWUgb3B0aW1pemVkIG91dCZndDssPGJyPiZuYnNwOyZuYnNwOyZu
YnNwOyBkZXNjPSZsdDt2YWx1ZSBvcHRpbWl6ZWQgb3V0Jmd0OywgdmFscz0weDk4Y2ViMCwgbnZh
bHM9MHg5NTNiZjApIGF0IGF0dHIuYzo0Nzc8YnI+IzQmbmJzcDsgMHgwMDAwMDAwMDAwNDY4MWJj
IGluIG1vZGlmeV9hZGRfdmFsdWVzIChlPTB4NDA4NWY3YjAsIG1vZD0weDk4Y2U3MCw8YnI+CiZu
YnNwOyZuYnNwOyZuYnNwOyBwZXJtaXNzaXZlPTAsIHRleHQ9MHg0MDg2MGNkMCwgdGV4dGJ1Zj0w
eDQwODVmOGIwICZxdW90OzDvv71cMjMwJnF1b3Q7LCB0ZXh0bGVuPTI1Nik8YnI+Jm5ic3A7Jm5i
c3A7Jm5ic3A7IGF0IG1vZHMuYzoxNTU8YnI+IzUmbmJzcDsgMHgwMDAwMDAwMDAwNDhlNGJmIGlu
IGhkYl9tb2RpZnlfaW50ZXJuYWwgKG9wPTB4OTUyNDAwLCB0aWQ9MHg5NTM2NDAsPGJyPiZuYnNw
OyZuYnNwOyZuYnNwOyBtb2RsaXN0PTB4OThjZTcwLCBlPTB4NDA4NWY3YjAsIHRleHQ9MHg0MDg2
MGNkMCw8YnI+CiZuYnNwOyZuYnNwOyZuYnNwOyB0ZXh0YnVmPTB4NDA4NWY4YjAgJnF1b3Q7MO+/
vVwyMzAmcXVvdDssIHRleHRsZW49MjU2KSBhdCBtb2RpZnkuYzoxMDE8YnI+IzYmbmJzcDsgMHgw
MDAwMDAwMDAwNDhlZWFkIGluIGhkYl9tb2RpZnkgKG9wPTB4OTUyNDAwLCBycz0weDQwODYwY2Iw
KTxicj4mbmJzcDsmbmJzcDsmbmJzcDsgYXQgbW9kaWZ5LmM6NTc4PGJyPiM3Jm5ic3A7IDB4MDAw
MDAwMDAwMDQzNDZlZCBpbiBmZV9vcF9tb2RpZnkgKG9wPTB4OTUyNDAwLCBycz0weDQwODYwY2Iw
KTxicj4KJm5ic3A7Jm5ic3A7Jm5ic3A7IGF0IG1vZGlmeS5jOjMwMDxicj4jOCZuYnNwOyAweDAw
MDAwMDAwMDA0MzRmZGEgaW4gZG9fbW9kaWZ5IChvcD0weDk1MjQwMCwgcnM9MHg0MDg2MGNiMCk8
YnI+Jm5ic3A7Jm5ic3A7Jm5ic3A7IGF0IG1vZGlmeS5jOjE3NTxicj4jOSZuYnNwOyAweDAwMDAw
MDAwMDA0MWUxOTMgaW4gY29ubmVjdGlvbl9vcGVyYXRpb24gKGN0eD0weDQwODYwZTAwLDxicj4m
bmJzcDsmbmJzcDsmbmJzcDsgYXJnX3Y9Jmx0O3ZhbHVlIG9wdGltaXplZCBvdXQmZ3Q7KSBhdCBj
b25uZWN0aW9uLmM6MTA4NDxicj4KIzEwIDB4MDAwMDAwMDAwMDQxZTk5NSBpbiBjb25uZWN0aW9u
X3JlYWRfdGhyZWFkIChjdHg9MHg0MDg2MGUwMCw8YnI+Jm5ic3A7Jm5ic3A7Jm5ic3A7IGFyZ3Y9
Jmx0O3ZhbHVlIG9wdGltaXplZCBvdXQmZ3Q7KSBhdCBjb25uZWN0aW9uLmM6MTIxMTxicj4jMTEg
MHgwMDAwMDAwMDAwNTQzMGRhIGluIGxkYXBfaW50X3RocmVhZF9wb29sX3dyYXBwZXIgKHhwb29s
PTB4OGQ3YzYwKTxicj4mbmJzcDsmbmJzcDsmbmJzcDsgYXQgdHBvb2wuYzo2NjM8YnI+CiMxMiAw
eDAwMDA3ZjcyNTZlNmQzZjcgaW4gc3RhcnRfdGhyZWFkICgpIGZyb20gL2xpYi9saWJwdGhyZWFk
LnNvLjA8YnI+IzEzIDB4MDAwMDdmNzI1NTk0ZmIyZCBpbiBjbG9uZSAoKSBmcm9tIC9saWIvbGli
Yy5zby42PGJyPiMxNCAweDAwMDAwMDAwMDAwMDAwMDAgaW4gPz8gKCk8YnI+PGJyPgo=
------=_Part_4694_18767492.1212106829556--



Followup 2

Download message
Date: Thu, 29 May 2008 17:41:08 -0700
From: Howard Chu <hyc@symas.com>
To: unix.gurus@gmail.com
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
unix.gurus@gmail.com wrote:
> ------=_Part_4694_18767492.1212106829556
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: base64
> Content-Disposition: inline
>
> VGhpcyBiYWNrdHJhY2UgbWF5IHByb3ZpZGUgc29tZSBmdXJ0aGVyIGluZm86CgojMCAgMHgwMDAw
> N2Y3MjU1OGFhMDk1IGluIHJhaXNlICgpIGZyb20gL2xpYi9saWJjLnNvLjYKIzEgIDB4MDAwMDdm
> NzI1NThhYmFmMCBpbiBhYm9ydCAoKSBmcm9tIC9saWIvbGliYy5zby42CiMyICAweDAwMDA3Zjcy
> NTU4YTMyZGYgaW4gX19hc3NlcnRfZmFpbCAoKSBmcm9tIC9saWIvbGliYy5zby42CiMzICAweDAw

Thanks, the original report was sufficient. It surfaces a larger problem, 
which we've been ignoring up till now. The correct action would be to iterate 
through all the databases and generate the normalized values for all the 
affected attributes. There is currently no code to make that happen though. 
(Likewise, if deleting matching rules, the normalized values must also be 
deleted.) As such, the only way to progress after such a change would be to 
dump and reload the DBs (slapcat/slapadd).

We'll probably need to discuss on the -devel list what steps to take from here.

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



Followup 3

Download message
Date: Fri, 30 May 2008 02:40:39 -0700
From: Howard Chu <hyc@symas.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
This is a multi-part message in MIME format.
--------------050600010007030200030106
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

hyc@symas.com wrote:
> We'll probably need to discuss on the -devel list what steps to take from
here.
>
I wrote the attached patch for the original issue. Unfortunately it asserted 
right away:

[Switching to Thread 1090525504 (LWP 23577)]
0x00002b55e738d535 in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x00002b55e738d535 in raise () from /lib64/libc.so.6
#1  0x00002b55e738e990 in abort () from /lib64/libc.so.6
#2  0x00002b55e7386c16 in __assert_fail () from /lib64/libc.so.6
#3  0x00000000004410e5 in attr_valadd (a=0xb866c8, vals=0x41000670, nvals=0x0, 
nn=1) at ../../../r24/servers/slapd/attr.c:394
#4  0x0000000000441a06 in attr_merge_one (e=0xb72c08, desc=0x8b3780, 
val=0x41000670, nval=0x0)
     at ../../../r24/servers/slapd/attr.c:599
#5  0x000000000043ea0f in slap_add_opattrs (op=0x965ec0, text=<value
optimized 
out>, textbuf=<value optimized out>,
     textlen=<value optimized out>, manage_ctxcsn=<value optimized
out>) at 
../../../r24/servers/slapd/add.c:664
#6  0x00000000004d9c42 in hdb_add (op=0x965ec0, rs=0x41000ca0) at add.c:108
#7  0x000000000043f1a6 in fe_op_add (op=0x965ec0, rs=0x41000ca0) at 
../../../r24/servers/slapd/add.c:334
#8  0x000000000043fa12 in do_add (op=0x965ec0, rs=0x41000ca0) at 
../../../r24/servers/slapd/add.c:194
#9  0x00000000004383a7 in connection_operation (ctx=0x41000de0, 
arg_v=0x965ec0) at ../../../r24/servers/slapd/connection.c:1084
#10 0x00000000004388cf in connection_read_thread (ctx=0x41000de0, argv=0xc) at 
../../../r24/servers/slapd/connection.c:1211
#11 0x000000000054b8ca in ldap_int_thread_pool_wrapper (xpool=0x8bf070) at 
../../../r24/libraries/libldap_r/tpool.c:663
#12 0x00002b55e655e09e in start_thread () from /lib64/libpthread.so.0
#13 0x00002b55e741e4cd in clone () from /lib64/libc.so.6
#14 0x0000000000000000 in ?? ()

It was adding the createTimestamp attribute, without providing normalized 
values. slap_add_opattrs was written before the generalizedTimeNormalize 
function was written... I suspect there will be a fair number of cases that 
need to be cleaned up. I don't have time at the moment to chase them all down. 
Anyone else want to jump in here? If not, we may have to push this back a bit. 
Note that this patch will probably require a fair number of databases to be 
reloaded.

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

--------------050600010007030200030106
Content-Type: text/plain;
 name="dif.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="dif.txt"

Index: attr.c
===================================================================
RCS file: /repo/OpenLDAP/pkg/ldap/servers/slapd/attr.c,v
retrieving revision 1.112.2.7
diff -u -r1.112.2.7 attr.c
--- attr.c	11 Feb 2008 23:26:43 -0000	1.112.2.7
+++ attr.c	30 May 2008 09:33:43 -0000
@@ -366,8 +366,39 @@
 	BerVarray nvals,
 	int nn )
 {
-	int		i;
 	BerVarray	v2;
+	int		i;
+
+	{
+		/* FIXME: if the schema has been edited, and an equality matching rule
+		 * has been added or removed from the attribute definition, the database
+		 * values may no longer be in sync with our expectations. Currently this
+		 * means the DB must be reloaded.
+		 */
+		int have_norm, have_nval, new_nval;
+		have_norm = a->a_desc->ad_type->sat_equality->smr_normalize !=
NULL;
+		have_nval = a->a_nvals != a->a_vals;
+		new_nval = nvals != NULL;
+
+		/* this check is only relevant if any values already exist */
+		if ( a->a_vals != NULL && have_norm != have_nval ) {
+			Debug(LDAP_DEBUG_ANY,
+				"attr_valadd: database inconsistent with schema definition of %s, reload
the DB\n",
+					a->a_desc->ad_cname.bv_val, 0, 0 );
+			return LDAP_OTHER;
+			/* no need to compare have_nval with new_nval; by transitivity they will
match if
+			 * the above check and the following check succeed.
+			 */
+		}
+
+		assert( have_norm == new_nval );
+		if ( have_norm != new_nval ) {
+			Debug(LDAP_DEBUG_ANY,
+				"attr_valadd: new values of %s not properly normalized (should never
happen!)\n",
+					a->a_desc->ad_cname.bv_val, 0, 0 );
+			return LDAP_OTHER;
+		}
+	}
 
 	v2 = (BerVarray) SLAP_REALLOC( (char *) a->a_vals,
 		    (a->a_numvals + nn + 1) * sizeof(struct berval) );
@@ -469,15 +500,6 @@
 
 	if ( *a == NULL ) {
 		*a = attr_alloc( desc );
-	} else {
-		/*
-		 * FIXME: if the attribute already exists, the presence
-		 * of nvals and the value of (*a)->a_nvals must be consistent
-		 */
-		assert( ( nvals == NULL && (*a)->a_nvals == (*a)->a_vals )
-				|| ( nvals != NULL && (
-					( (*a)->a_vals == NULL && (*a)->a_nvals == NULL )
-					|| ( (*a)->a_nvals != (*a)->a_vals )

Message of length 5081 truncated


Followup 4

Download message
Date: Fri, 30 May 2008 10:14:58 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: hyc@symas.com
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Cc: openldap-its@openldap.org
------=_Part_7327_2415938.1212167698754
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

Hi,

On Fri, May 30, 2008 at 2:40 AM, <hyc@symas.com> wrote:

> This is a multi-part message in MIME format.
> --------------050600010007030200030106
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed
> Content-Transfer-Encoding: 7bit
>
> hyc@symas.com wrote:
> > We'll probably need to discuss on the -devel list what steps to take
from
> here.
> >
> I wrote the attached patch for the original issue. Unfortunately it
> asserted
> right away:
>
> [Switching to Thread 1090525504 (LWP 23577)]
> 0x00002b55e738d535 in raise () from /lib64/libc.so.6
> (gdb) bt
> #0  0x00002b55e738d535 in raise () from /lib64/libc.so.6
> #1  0x00002b55e738e990 in abort () from /lib64/libc.so.6
> #2  0x00002b55e7386c16 in __assert_fail () from /lib64/libc.so.6
> #3  0x00000000004410e5 in attr_valadd (a=0xb866c8, vals=0x41000670,
> nvals=0x0,
> nn=1) at ../../../r24/servers/slapd/attr.c:394
> #4  0x0000000000441a06 in attr_merge_one (e=0xb72c08, desc=0x8b3780,
> val=0x41000670, nval=0x0)
>     at ../../../r24/servers/slapd/attr.c:599
> #5  0x000000000043ea0f in slap_add_opattrs (op=0x965ec0, text=<value
> optimized
> out>, textbuf=<value optimized out>,
>     textlen=<value optimized out>, manage_ctxcsn=<value optimized
out>) at
> ../../../r24/servers/slapd/add.c:664
> #6  0x00000000004d9c42 in hdb_add (op=0x965ec0, rs=0x41000ca0) at add.c:108
> #7  0x000000000043f1a6 in fe_op_add (op=0x965ec0, rs=0x41000ca0) at
> ../../../r24/servers/slapd/add.c:334
> #8  0x000000000043fa12 in do_add (op=0x965ec0, rs=0x41000ca0) at
> ../../../r24/servers/slapd/add.c:194
> #9  0x00000000004383a7 in connection_operation (ctx=0x41000de0,
> arg_v=0x965ec0) at ../../../r24/servers/slapd/connection.c:1084
> #10 0x00000000004388cf in connection_read_thread (ctx=0x41000de0, argv=0xc)
> at
> ../../../r24/servers/slapd/connection.c:1211
> #11 0x000000000054b8ca in ldap_int_thread_pool_wrapper (xpool=0x8bf070) at
> ../../../r24/libraries/libldap_r/tpool.c:663
> #12 0x00002b55e655e09e in start_thread () from /lib64/libpthread.so.0
> #13 0x00002b55e741e4cd in clone () from /lib64/libc.so.6
> #14 0x0000000000000000 in ?? ()
>
> It was adding the createTimestamp attribute, without providing normalized
> values. slap_add_opattrs was written before the generalizedTimeNormalize
> function was written... I suspect there will be a fair number of cases that
> need to be cleaned up. I don't have time at the moment to chase them all
> down.
> Anyone else want to jump in here? If not, we may have to push this back a
> bit.
> Note that this patch will probably require a fair number of databases to be
> reloaded.
>
> --
>   -- Howard Chu
>   CTO, Symas Corp.           http://www.symas.com
>   Director, Highland Sun     http://highlandsun.com/hyc/
>   Chief Architect, OpenLDAP  http://www.openldap.org/project/
>
> --------------050600010007030200030106
> Content-Type: text/plain;
>  name="dif.txt"
> Content-Transfer-Encoding: 7bit
> Content-Disposition: inline;
>  filename="dif.txt"
>
> Index: attr.c
> ===================================================================
> RCS file: /repo/OpenLDAP/pkg/ldap/servers/slapd/attr.c,v
> retrieving revision 1.112.2.7
> diff -u -r1.112.2.7 attr.c
> --- attr.c      11 Feb 2008 23:26:43 -0000      1.112.2.7
> +++ attr.c      30 May 2008 09:33:43 -0000
> @@ -366,8 +366,39 @@
>        BerVarray nvals,
>        int nn )
>  {
> -       int             i;
>        BerVarray       v2;
> +       int             i;
> +
> +       {
> +               /* FIXME: if the schema has been edited, and an equality
> matching rule
> +                * has been added or removed from the attribute definition,
> the database
> +                * values may no longer be in sync with our expectations.
> Currently this
> +                * means the DB must be reloaded.
> +                */
> +               int have_norm, have_nval, new_nval;
> +               have_norm =
a->a_desc->ad_type->sat_equality->smr_normalize
> != NULL;


Not all attributes have an equality matching rule.  This causes your patch
to segfault since sat_equality is NULL:

> have_norm = a->a_desc->ad_type->sat_equality->smr_normalize !=
NULL;
>

This seems to be a better way to generate have_norm:

> have_norm = a->a_desc->ad_type->sat_equality != NULL &&
>           a->a_desc->ad_type->sat_equality->smr_normalize !=
NULL;
>

Once that is fixed the config backend causes an assert for me when it
attempts to add createTimestamp, which is exactly the p

Message of length 21821 truncated


Followup 5

Download message
Date: Fri, 30 May 2008 10:51:23 -0700
From: Howard Chu <hyc@symas.com>
To: Sean Burford <unix.gurus@gmail.com>
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Sean Burford wrote:
> Not all attributes have an equality matching rule. This causes your
> patch to segfault since sat_equality is NULL:
>
>     have_norm = a->a_desc->ad_type->sat_equality->smr_normalize
!= NULL;
>
>
> This seems to be a better way to generate have_norm:
>
>     have_norm = a->a_desc->ad_type->sat_equality != NULL
&&
>     a->a_desc->ad_type->sat_equality->smr_normalize != NULL;

Thanks, looks fine.

> Once that is fixed the config backend causes an assert for me when it
> attempts to add createTimestamp, which is exactly the problem you describe.
>
> I'll create a patch that addresses any of these normalization issues
> that I can identify and send it to one of the openldap lists. Would
> openldap-its or openldap-devel be preferable?

Just continue to reply to this ITS for now.
-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/



Followup 6

Download message
Date: Fri, 30 May 2008 16:49:15 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: hyc@symas.com
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Cc: openldap-its@openldap.org
Hi,

On Fri, May 30, 2008 at 2:40 AM, <hyc@symas.com> wrote:
>
> I wrote the attached patch for the original issue. Unfortunately it
asserted
> right away:
...
> It was adding the createTimestamp attribute, without providing normalized
> values. slap_add_opattrs was written before the generalizedTimeNormalize
> function was written... I suspect there will be a fair number of cases that
> need to be cleaned up. I don't have time at the moment to chase them all
down.
> Anyone else want to jump in here? If not, we may have to push this back a
bit.

I've modified my copy of the source to normalize data where needed.
slapd starts up and my test can run.  With the old assertion in place
it fails exactly as it did before, because the old attr_merge()
assertion runs before your new attr_valadd() normalization check.

Removing the old attr_merge() assertion results in your new
attr_valadd() check being triggered.  Removing the old assertion looks
safe, since attr_merge() calls attr_valadd() almost immediately after
the assertion anyway.

With these changes, running my test script from the original tarball results in:
bdb_modify_internal: add testAttribute
attr_valadd: database inconsistent with schema definition of
testAttribute, reload the DB
bdb_modify_internal: 80 modify/add: testAttribute: merge error (80)
hdb_modify: modify failed (80)

I'm working through 'make test' now to find more instances that need
fixing.  After that I'll send some patches.

> Note that this patch will probably require a fair number of databases to be
> reloaded.

I'm not even sure that attributes can be automatically renormalized
when a problem is detected, since bugs (eg. with the generation of
timestamps, CSNs or referalls) can also trigger the normalization
checks.

--
Thanks,
Sean Burford



Followup 7

Download message
Date: Fri, 30 May 2008 18:32:05 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
I'm having some trouble working out where to set the normalized value
for ContextCSN in the modlist for the syncrepl refresh consumer
(test017-syncreplication-refresh fails).

The assertion backtrace looks like this:
#0  0x00007f472879d095 in raise () from /lib/libc.so.6
#1  0x00007f472879eaf0 in abort () from /lib/libc.so.6
#2  0x00007f47287962df in __assert_fail () from /lib/libc.so.6
#3  0x0000000000425903 in attr_valadd (a=0x8fb380, vals=0xc72130,
nvals=0x0, nn=1) at attr.c:398
#4  0x000000000046714c in modify_add_values (e=0x41ff5bb0,
mod=0x41ff60e0, permissive=0, text=0x41ff6090,
    textbuf=0x41ff5cb0 "", textlen=256) at mods.c:155
#5  0x000000000048147d in bdb_modify_internal (op=0x41ff6780,
tid=0xc71f50, modlist=0x41ff60e0, e=0x41ff5bb0,
    text=0x41ff6090, textbuf=0x41ff5cb0 "", textlen=256) at modify.c:133
#6  0x0000000000481f6d in bdb_modify (op=0x41ff6780, rs=0x41ff6070) at
modify.c:578
#7  0x0000000000477d32 in overlay_op_walk (op=0x41ff6780,
rs=0x41ff6070, which=op_modify, oi=0x858990, on=0x0)
    at backover.c:646
#8  0x00000000004782b3 in over_op_func (op=0x41ff6780, rs=0x41ff6070,
which=op_modify) at backover.c:698
#9  0x000000000046dc59 in syncrepl_updateCookie (si=0x858560,
op=0x249e, pdn=<value optimized out>, syncCookie=0x41ff6310)
    at syncrepl.c:2785
#10 0x0000000000472bea in do_syncrep2 (op=0x41ff6780, si=0x858560) at
syncrepl.c:961
#11 0x0000000000475072 in do_syncrepl (ctx=0x0, arg=0x8588f0) at syncrepl.c:1276
#12 0x00000000004daa4a in ldap_int_thread_pool_wrapper
(xpool=0x82e440) at tpool.c:663
#13 0x00007f47296d03f7 in start_thread () from /lib/libpthread.so.0
#14 0x00007f4728842b2d in clone () from /lib/libc.so.6
#15 0x0000000000000000 in ?? ()

In frame 3 a->a_desc->ad_type->sat_equality has a value but nvals has
not been set, which is why the assertion triggers:
(gdb) print a->a_desc->ad_type->sat_equality
$39 = (MatchingRule *) 0x821be0

Way back in frame 10 do_syncrepl passes a modlist containing an
unnormalized contextCSN to do_syncrep2, leading me to believe that
I've missed something in syncrepl.c:
(gdb) print *op->o_request->oq_modify->rs_mods->rs_modlist
$44 = {sml_mod = {sm_desc = 0x824b70, sm_values = 0xc72130, sm_nvalues
= 0x0, sm_numvals = 1, sm_op = 2, sm_flags = 0, sm_type = {bv_len =
10, bv_val = 0x824be0 "contextCSN"}}, sml_next = 0x0}

The debug output up to the assertion is:
slap_queue_csn: queing 0xc72ae0 20080531010928.580166Z#000000#000#000000
daemon: epoll: listen=10 active_threads=0 tvp=zero
=> bdb_entry_get: ndn: "dc=example,dc=com"
=> bdb_entry_get: oc: "(null)", at: "(null)"
bdb_dn2entry("dc=example,dc=com")
=> bdb_entry_get: found entry: "dc=example,dc=com"
bdb_entry_get: rc=0
bdb_modify: dc=example,dc=com
bdb_dn2entry("dc=example,dc=com")
bdb_modify_internal: 0x00000001: dc=example,dc=com
<= acl_access_allowed: granted to database root
bdb_modify_internal: replace contextCSN
slapd: attr.c:398: attr_valadd: Assertion `have_norm == new_nval' failed.

-- 
Thanks,
Sean Burford



Followup 8

Download message
Date: Sat, 31 May 2008 06:54:23 -0700
From: Howard Chu <hyc@symas.com>
To: Sean Burford <unix.gurus@gmail.com>
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Sean Burford wrote:
> Hi,
>
> On Fri, May 30, 2008 at 2:40 AM,<hyc@symas.com>  wrote:
>> I wrote the attached patch for the original issue. Unfortunately it
asserted
>> right away:
> ...
>> It was adding the createTimestamp attribute, without providing
normalized
>> values. slap_add_opattrs was written before the
generalizedTimeNormalize
>> function was written... I suspect there will be a fair number of cases
that
>> need to be cleaned up. I don't have time at the moment to chase them
all down.
>> Anyone else want to jump in here? If not, we may have to push this back
a bit.
>
> I've modified my copy of the source to normalize data where needed.
> slapd starts up and my test can run.  With the old assertion in place
> it fails exactly as it did before, because the old attr_merge()
> assertion runs before your new attr_valadd() normalization check.

That aspect of the patch is not optional. The old check is in the wrong place 
and must be removed. It misses the other places where attrs are being added, 
while attr_valadd catches all of them.

> Removing the old attr_merge() assertion results in your new
> attr_valadd() check being triggered.  Removing the old assertion looks
> safe, since attr_merge() calls attr_valadd() almost immediately after
> the assertion anyway.
>
> With these changes, running my test script from the original tarball
results in:
> bdb_modify_internal: add testAttribute
> attr_valadd: database inconsistent with schema definition of
> testAttribute, reload the DB
> bdb_modify_internal: 80 modify/add: testAttribute: merge error (80)
> hdb_modify: modify failed (80)
>
> I'm working through 'make test' now to find more instances that need
> fixing.  After that I'll send some patches.
>
>> Note that this patch will probably require a fair number of databases
to be
>> reloaded.
>
> I'm not even sure that attributes can be automatically renormalized
> when a problem is detected, since bugs (eg. with the generation of
> timestamps, CSNs or referalls) can also trigger the normalization
> checks.

I think it's important to continue to assert as in my patch, at least at this 
stage, so we can catch all of those bugs. I also don't think we can silently 
renormalize just the cases that the "reload the DB" message detects, because 
that's obviously only going to happen on entries that are explicitly modified, 
and the rest of the DB will still have problems.

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



Followup 9

Download message
Date: Wed, 4 Jun 2008 13:08:55 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: hyc@symas.com
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Cc: openldap-its@openldap.org
HI,

On Fri, May 30, 2008 at 2:40 AM,  <hyc@symas.com> wrote:
> hyc@symas.com wrote:
>> We'll probably need to discuss on the -devel list what steps to take
from here.
>>
> I wrote the attached patch for the original issue. Unfortunately it
asserted
> right away:
>
...
>
> It was adding the createTimestamp attribute, without providing normalized
> values. slap_add_opattrs was written before the generalizedTimeNormalize
> function was written... I suspect there will be a fair number of cases that
> need to be cleaned up.

I've identified these attributes as having differences between their
nval population and their schema.

I've been using Google coredumper instead of assertions to mark places
where normalization looks bad.  In this way, I get a bunch of core
files to look through after a run instead of a single assertion.  Once
all of the attributes that have bad normalization have been identified
in this way, I'll go back over the source and look for other places
where they are used.

I haven't found all of the normalization differences yet, but I wanted
to check in to ensure I'm on the right path.  I would like your
comments on whether these look like the right changes to make.  You
can refer to ftp://ftp.openldap.org/incoming/sburford-initial-normalization-20080604.patch
for the details.

createTimestamp
createTimestamp does have an equality matching rule, as I think it should.
The buffer timestamp is populated with a generalized time by
slap_timestamp(), so I add &timestamp as the nval for the calls to
attr_merge_one in add.c slap_add_opattrs() and schema.c schema_info().
 I also use a slight variation of this for createTimestamp in
slapadd.c slapadd().

modifyTimestamp
Same arguments and modifications as createTimestamp.
modifyTimestamp is also used in modify.c slap_mods_opattrs(), where I
ch_malloc a new BerVarray for the mod->sml_nvalues.  I dont use the
same BerVarray for vals and nvals in case whatever frees mod entries
frees them as separate pointers (double free).  Does something free
all of the vals and nvals in the modify list?

entryCSN
entryCSN does have an equality matching rule, as I think it should.
In add.c slap_add_opattrs() I change the attr_merge_one into an
attr_merge_normalize_one with op->o_tmpmemctx as the memory context.
I haven't looked into the memory context argument, but assume it
contains a list of things to free when the context is finished with?
In slapadd I also change the attr_merge_one into an
attr_merge_normalize_one but I pass a NULL memory context, since
slapadd is short lived and there are other examples like this in the
nearby source code.
In modify.c slap_mods_opattrs() I ch_malloc a new BerVarray for the
entryCSN nval and use attr_normalize_one to populate it.  Again I
assume something frees the components of the modify list later.

contextCSN
contextCSN does have an equality matching rule, as I think it should.
In ctxcsn.c slap_create_context_csn_entr() I change attr_merge_one
into attr_merge_normalize_one with a NULL memory context.
In overlays/syncprov.c syncprov_checkpoint() and syncrepl.c
syncrepl_updateCookie() it is a bit more difficult because a local
modlist is created without any attr_merge calls, so I ch_malloc an
array for the nvals, and populate it with attr_normalize and use
ber_array_bvfree to free it.

monitorCounter
monitorCounter does not have an equality matching rule, so I remove
the nval argument from attr_merge_one() in back-monitor/conn.c
monitor_subsys_conn_init() (affects counters for max FD, total conns
and current conns).

monitorOpCompleted
monitorOpInitiated
These attributes do not have equality matching rules, so I remove the
nval argument from attr_merge_one() in back-monitor/operation.c
monitor_subsys_ops_init().

monitorContext
namingContexts
configContext
dynamicSubtrees
These attributes do not have equality matching rules, but probably
should.  I add 'EQUALITY distinguishedNameMatch' to them in
schema_prep.c.

olcRootDN
olcSchemaDN
These attributes do not have equality matching rules, but probably
should.  I add 'EQUALITY distinguishedNameMatch' to them in bconfig.c.

supportedControl
This attribute does not have an equality matching rule, but probably
should.  I add 'EQUALITY objectIdentifierMatch' to it in
schema_prep.c.

readOnly
This attribute does not have an equality matching rule, I don't know
if it should.  I NULLed the nval passed to attr_merge_one in
back-monitor/database.c.

default_referral:
In rootdse.c root_dse_info() I change attr_merge to
attr_merge_normalize for default_referral, since it has an equality
matching rule.  There doesn't seem to be a memory context in use here.
 I suspect this normalization might result in memory leakage unless
followed by a free in root_dse_info()?

Also, bconfig.c config_build_attrs() contains this logic which calls
attr_merge_normalize() for values that don't need normalization.
attr_merge_normalize won't normalize them though

Message of length 5635 truncated


Followup 10

Download message
From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
Date: Wed, 4 Jun 2008 23:20:04 +0200
To: unix.gurus@gmail.com
Cc: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
I haven't follwed this ITS, but a few notes anyway:

unix.gurus@gmail.com writes:
> monitorCounter
> monitorCounter does not have an equality matching rule,

Yes it does.  back-monitor/init.c line 1710.  What are you looking at?
(Hmm, I too had the impression it had no EQUALITY rule...)

> monitorOpCompleted
> monitorOpInitiated
> These attributes do not have equality matching rules,

These do too, inherited from monitorCounter.
it doesn't work right in 2.3 however:

ldapsearch -LLL -bcn=monitor -s one '(monitorCounter=0)' monitorCounter
dn: cn=Operations,cn=Monitor
monitorOpInitiated: 231542983
monitorOpCompleted: 231542982

> namingContexts
> configContext
> dynamicSubtrees
>
> These attributes do not have equality matching rules, but probably
> should.  I add 'EQUALITY distinguishedNameMatch' to them in
> schema_prep.c.

No, that breaks the definitions in the RFCs they come from.  (See their
DESC strings.)  I don't know why they lack EQUALITY rules, maybe we just
forgot when we defined them, but that's the way it is.  Same with
supportedControl.

monitorContext, readOnly and the olc* attributes are defined by OpenLDAP
(their OIDs start with 1.3.6.1.4.1.4203) and can be modified if we feel
like it.  Personally I prefer attrs to have the matching rules they can
have unless there is a reason not to, but I didn't write these modules
so I don't know if there _is_ a reason not to.

-- 
Hallvard



Followup 11

Download message
Date: Wed, 4 Jun 2008 16:52:01 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: "Hallvard B Furuseth" <h.b.furuseth@usit.uio.no>
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Cc: openldap-its@openldap.org
Hi,

On Wed, Jun 4, 2008 at 2:20 PM, Hallvard B Furuseth
<h.b.furuseth@usit.uio.no> wrote:
> I haven't follwed this ITS, but a few notes anyway:
>
> unix.gurus@gmail.com writes:
>> monitorCounter
>> monitorCounter does not have an equality matching rule,
>
> Yes it does.  back-monitor/init.c line 1710.  What are you looking at?
> (Hmm, I too had the impression it had no EQUALITY rule...)

I'm working with OpenLDAP 2.4.9.

You're right.  integerMatch has an equality rule but no normalizing
function.  The logic that determines whether a normalized value should
exist says:
    have_norm = a->a_desc->ad_type->sat_equality != NULL &&

a->a_desc->ad_type->sat_equality->smr_normalize != NULL;
...
    new_nval = nvals != NULL;
...
if ( have_norm != new_nval ) {
    assert()

So we don't need nvals for integers.  For this reason I was right to
remove the nval for monitorCounter, monitorOpInitiated,
monitorOpCompleted, but my reasoning was wrong.

>> monitorOpCompleted
>> monitorOpInitiated
>> These attributes do not have equality matching rules,
>
> These do too, inherited from monitorCounter.

See above.

> it doesn't work right in 2.3 however:
>
> ldapsearch -LLL -bcn=monitor -s one '(monitorCounter=0)' monitorCounter
> dn: cn=Operations,cn=Monitor
> monitorOpInitiated: 231542983
> monitorOpCompleted: 231542982

I wonder if val is being maintained and nval is being left at 0.  The
code at the end of monitor_subsys_ops_update() maintains a_vals but
not a_nvals, so that is probably the problem:
        /* NOTE: no minus sign is allowed in the counters... */
        UI2BV( &a->a_vals[ 0 ], nInitiated );
        ldap_pvt_mp_clear( nInitiated );
...
        /* NOTE: no minus sign is allowed in the counters... */
        UI2BV( &a->a_vals[ 0 ], nCompleted );
        ldap_pvt_mp_clear( nCompleted );

>> namingContexts
>> configContext
>> dynamicSubtrees
>>
>> These attributes do not have equality matching rules, but probably
>> should.  I add 'EQUALITY distinguishedNameMatch' to them in
>> schema_prep.c.
>
> No, that breaks the definitions in the RFCs they come from.  (See their
> DESC strings.)  I don't know why they lack EQUALITY rules, maybe we just
> forgot when we defined them, but that's the way it is.  Same with
> supportedControl.

Ok, so we should not define nvals for them?

> monitorContext, readOnly and the olc* attributes are defined by OpenLDAP
> (their OIDs start with 1.3.6.1.4.1.4203) and can be modified if we feel
> like it.  Personally I prefer attrs to have the matching rules they can
> have unless there is a reason not to, but I didn't write these modules
> so I don't know if there _is_ a reason not to.

-- 
Thanks,
Sean Burford



Followup 12

Download message
Date: Thu, 12 Jun 2008 12:34:53 -0700
From: Howard Chu <hyc@symas.com>
To: unix.gurus@gmail.com
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
unix.gurus@gmail.com wrote:
>>> namingContexts
>>> configContext
>>> dynamicSubtrees
>>>
>>> These attributes do not have equality matching rules, but probably
>>> should.  I add 'EQUALITY distinguishedNameMatch' to them in
>>> schema_prep.c.
>> No, that breaks the definitions in the RFCs they come from.  (See their
>> DESC strings.)  I don't know why they lack EQUALITY rules, maybe we
just
>> forgot when we defined them, but that's the way it is.  Same with
>> supportedControl.

configContext is OpenLDAP-specific. It is also single-valued, so it seemed to 
me it did not need an EQ rule.

> Ok, so we should not define nvals for them?

No normalizer, therefore, cannot provide nval...

>> monitorContext, readOnly and the olc* attributes are defined by
OpenLDAP
>> (their OIDs start with 1.3.6.1.4.1.4203) and can be modified if we feel
>> like it.  Personally I prefer attrs to have the matching rules they can
>> have unless there is a reason not to, but I didn't write these modules
>> so I don't know if there _is_ a reason not to.

For the config attributes, I just assumed no one needs to search on them (so 
no filter capability needed) and for single-valued attributes, there's no need 
to consider modifies...

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



Followup 13

Download message
From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
Date: Thu, 12 Jun 2008 23:13:23 +0200
To: hyc@symas.com
Cc: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
hyc@symas.com writes:
>>> monitorContext, readOnly and the olc* attributes are defined by
OpenLDAP
>>> (their OIDs start with 1.3.6.1.4.1.4203) and can be modified if we
feel
>>> like it.  Personally I prefer attrs to have the matching rules they
can
>>> have unless there is a reason not to, but I didn't write these
modules
>>> so I don't know if there _is_ a reason not to.
>
> For the config attributes, I just assumed no one needs to search on
> them (so no filter capability needed) and for single-valued
> attributes, there's no need to consider modifies...

Oh, good.  No reason not to add matching rules then:-)

I search for one config attrs or another once in a while.  Mostly for
olcSuffix and with presence filters, but it'd be nice if e.g.
(olcAccess:caseIgnoreSubstringsMatch:=*userPassword*) worked.  Also some
day I'll want to use assertion controls as a check on modify operations
someday.

Still, I'm not proposing to add more work than necessary to this ITS.
Get it right first, then think of "nice to have"-features if/when anyone
bothers.

-- 
Hallvard



Followup 14

Download message
Date: Tue, 24 Jun 2008 12:50:43 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Hi,

I have uploaded a patch against HEAD that normalizes the attributes
used under cn=monitor according to the schema:
ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-080624.patch

I picked cn=monitor for the first upload since it can be modified
without having to reimport databases.  Once we've worked through any
issues with this patch I'll seperate out the rest of the normalization
patches and send those in.

namingContexts was the trickiest, since RFC 4512 defines it without an
equality matching rule but monitor_back_register_database() compares
it with backend suffixes.  It has to deal with capitalised values
properly otherwise we end up with duplicate namingContexts entries, so
normalization is required.  The relevant part of the diff is below.  I
am open to suggestions about how this should be dealt with:

+             struct berval   nContext;
+
+             for ( j = 0; !BER_BVISNULL( &a->a_vals[ j ] ); j++ ) {
+                     /* FIXME: namingContexts has no equality rule
+                      * but it needs to be normalized for this
+                      * comparison.  Using si_ad_distinguishedName
+                      * instead of si_ad_namingContexts results
+                      * in the normalization we expect for now.
+                      */
+                     if ( attr_normalize_one(
slap_schema.si_ad_distinguishedName,
+                                             &a->a_vals[ j ],
+                                             &nContext,
+                                             NULL ) ) {
+                             Debug( LDAP_DEBUG_ANY,
+                                     "monitor_subsys_database_init: "
+                                     "unable to normalize DN %d \"%s\"\n",
+                                     j, a->a_vals[ j ], 0 );
+                             return( -1 );
+                     }

                      for ( k = 0; !BER_BVISNULL( &be->be_nsuffix[ k ]
); k++ ) {
!                             if ( dn_match( &nContext,
&be->be_nsuffix[ k ] ) ) {
                                     rc = 0;
                                     goto done;
                               }
                      }
+                     free( nContext.bv_val );


The attribute types that are modified and the attributes that are
affected are listed below.  cn=Monitor didn't need and schema
modifications:

monitorContext: removed nvals
namingContexts: removed nvals
rootDSE monitorContext and namingContexts

monitorCounter: removed nvals
cn=Max File Descriptors
cn=Total,cn=Connections
cn=Current,cn=Connections
cn=Read under rww
DN counter under sent

monitorConnectionProtocol: removed nvals
monitorConnectionMask: normalized
monitorConnectionListener: normalized
monitorConnectionPeerDomain: normalized
monitorConnectionPeerAddress: normalized
monitorConnectionLocalAddress: normalized
monitorConnectionStartTime: normalized
monitorConnectionActivityTime: normalized
monitorOpInitiated: removed nvals
monitorOpCompleted: removed nvals

readOnly: removed nvals

-- 
Thanks,
Sean Burford



Followup 15

Download message
Date: Tue, 24 Jun 2008 15:37:49 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
On Tue, Jun 24, 2008 at 12:50 PM,  <unix.gurus@gmail.com> wrote:
> Hi,
>
> I have uploaded a patch against HEAD that normalizes the attributes
> used under cn=monitor according to the schema:
> ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-080624.patch

Unified diff of the same thing:
sean-burford-monitor-normalize-unified-080624.patch

The search (namingContexts:distinguishedNameMatch:=CN=...) returns the
expected results, I'll check what filterentry.c:test_mra_filter() is
up to later.

make test also completes successfully, for what it's worth.

-- 
Thanks,
Sean Burford



Followup 16

Download message
Date: Wed, 25 Jun 2008 13:24:11 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Hi,

On Tue, Jun 24, 2008 at 3:37 PM,  <unix.gurus@gmail.com> wrote:
> On Tue, Jun 24, 2008 at 12:50 PM,  <unix.gurus@gmail.com> wrote:
>> Hi,
>>
>> I have uploaded a patch against HEAD that normalizes the attributes
>> used under cn=monitor according to the schema:
>> ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-080624.patch
>
> Unified diff of the same thing:
> sean-burford-monitor-normalize-unified-080624.patch
>
> The search (namingContexts:distinguishedNameMatch:=CN=...) returns the
> expected results, I'll check what filterentry.c:test_mra_filter() is
> up to later.

As far as I can tell the arguments passed to
filterentry.c:test_mra_filter() for that search also look sane, so
whilst it passes my tests I'm looking forward to an official opinion
on this patch.

> make test also completes successfully, for what it's worth.

-- 
Thanks,
Sean Burford



Followup 17

Download message
Date: Sat, 28 Jun 2008 18:00:34 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
I've uploaded a tiny patch that checks the return code from
structural_class() in config_build_entry() and outputs an error
message if it fails.  This makes it easier to debug structural_class()
failures before too much other code is run:
ftp://ftp.openldap.org/incoming/sean-burford-structural-error-080628.patch

-- 
Thanks,
Sean Burford



Followup 18

Download message
Date: Sat, 28 Jun 2008 18:32:42 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
I've uploaded a patch that adds an equality matching rule to the
schema for olcRootDN and olcSchemaDN so that the normalization matches
the schema.  These attributes are already normalized wherever they are
generated, and their normalized values are used in bvmatches, so
removing the nvals instead of modifying the schema would result extra
normalization calls later.
ftp://ftp.openldap.org/incoming/sean-burford-rootdn-080628.patch

If someone wants to accept or comment on what needs fixing in the
patch below, that would help me to generate the rest of the patches:
ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-unified-080624.patch

-- 
Thanks,
Sean Burford



Followup 19

Download message
Date: Sat, 28 Jun 2008 19:09:50 -0700
From: Howard Chu <hyc@symas.com>
To: unix.gurus@gmail.com
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
unix.gurus@gmail.com wrote:
> I've uploaded a patch that adds an equality matching rule to the
> schema for olcRootDN and olcSchemaDN so that the normalization matches
> the schema.  These attributes are already normalized wherever they are
> generated, and their normalized values are used in bvmatches, so
> removing the nvals instead of modifying the schema would result extra
> normalization calls later.
> ftp://ftp.openldap.org/incoming/sean-burford-rootdn-080628.patch

Committed, thanks.

> If someone wants to accept or comment on what needs fixing in the
> patch below, that would help me to generate the rest of the patches:
> ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-unified-080624.patch

If it makes life easier, we can add the DN eq rule to monitorContext and 
configContext. Since they're OpenLDAP-specific attributes we can make this 
change without any repercussions.

IMO, it really needs to be added to namingContexts since that's multivalued.
-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/



Followup 20

Download message
Date: Sat, 28 Jun 2008 19:45:11 -0700
From: Howard Chu <hyc@symas.com>
To: unix.gurus@gmail.com
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
unix.gurus@gmail.com wrote:
> If someone wants to accept or comment on what needs fixing in the
> patch below, that would help me to generate the rest of the patches:
> ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-unified-080624.patch

In back-monitor/database.c you don't need to normalize the DNs before 
comparing them. The stored values are already Prettied, so all you need is to 
use a case-insensitive compare here.
-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/



Followup 21

Download message
Date: Sat, 28 Jun 2008 19:47:00 -0700
From: Howard Chu <hyc@symas.com>
To: unix.gurus@gmail.com
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Howard Chu wrote:
> unix.gurus@gmail.com wrote:
>> If someone wants to accept or comment on what needs fixing in the
>> patch below, that would help me to generate the rest of the patches:
>> ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-unified-080624.patch
>
> In back-monitor/database.c you don't need to normalize the DNs before
> comparing them. The stored values are already Prettied, so all you need is
to
> use a case-insensitive compare here.

Duh. Or just compare a->a_vals and be->be_suffix instead.

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



Followup 22

Download message
Date: Sun, 6 Jul 2008 18:06:43 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: "Howard Chu" <hyc@symas.com>
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Cc: openldap-its@openldap.org
Hi,

On Sat, Jun 28, 2008 at 7:47 PM, Howard Chu <hyc@symas.com> wrote:
> Howard Chu wrote:
>> unix.gurus@gmail.com wrote:
>>> If someone wants to accept or comment on what needs fixing in the
>>> patch below, that would help me to generate the rest of the
patches:
>>>
>>> ftp://ftp.openldap.org/incoming/sean-burford-monitor-normalize-unified-080624.patch
>>
>> In back-monitor/database.c you don't need to normalize the DNs before
>> comparing them. The stored values are already Prettied, so all you need
is
>> to
>> use a case-insensitive compare here.
>
> Duh. Or just compare a->a_vals and be->be_suffix instead.

Good point.  I've uploaded the following patch which does that as well
as adding matching rules to configContext and monitorContext:
ftp://ftp.openldap.org/sean-burford-monitor-normalize-unified-080706.patch

This patch should make the handling of normalization within cn=monitor
consistent.

-- 
Thanks,
Sean Burford



Followup 23

Download message
Date: Sun, 6 Jul 2008 18:22:10 -0700
From: "Sean Burford" <unix.gurus@gmail.com>
To: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
Hi,

On Tue, Jun 24, 2008 at 12:50 PM, Sean Burford <unix.gurus@gmail.com>
wrote:
> namingContexts was the trickiest, since RFC 4512 defines it without an
> equality matching rule but monitor_back_register_database() compares
> it with backend suffixes.  It has to deal with capitalised values
> properly otherwise we end up with duplicate namingContexts entries, so
> normalization is required.

I have uploaded
ftp://ftp.openldap.org/incoming/sean-burford-monitor-test-080706.patch
which adds a test for various things under cn=monitor.  If duplicate
namingContexts creep in (as mentioned above) it detects that.

The duplicate naming contexts thing was fixed by comparing a_vals with
be_suffix, I just wrote the test since there was no existing
cn=monitor test.

-- 
Thanks,
Sean Burford



Followup 24

Download message
Date: Tue, 27 Jan 2009 01:20:49 -0800
From: Howard Chu <hyc@symas.com>
To: unix.gurus@gmail.com
CC: openldap-its@openldap.org
Subject: Re: (ITS#5540) Normalization assertion in attr.c
unix.gurus@gmail.com wrote:
> Hi,
>
> On Tue, Jun 24, 2008 at 12:50 PM, Sean Burford<unix.gurus@gmail.com> 
wrote:
>> namingContexts was the trickiest, since RFC 4512 defines it without an
>> equality matching rule but monitor_back_register_database() compares
>> it with backend suffixes.  It has to deal with capitalised values
>> properly otherwise we end up with duplicate namingContexts entries, so
>> normalization is required.
>
> I have uploaded
> ftp://ftp.openldap.org/incoming/sean-burford-monitor-test-080706.patch
> which adds a test for various things under cn=monitor.  If duplicate
> namingContexts creep in (as mentioned above) it detects that.
>
> The duplicate naming contexts thing was fixed by comparing a_vals with
> be_suffix, I just wrote the test since there was no existing
> cn=monitor test.

Sorry for dropping the ball on this. These last two patches are now committed 
in HEAD.

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


Up to top level
Build   Contrib   Development   Documentation   Historical   Incoming   Software Bugs   Software Enhancements   Web  

Logged in as guest


The OpenLDAP Issue Tracking System uses a hacked version of JitterBug

______________
© Copyright 2013, OpenLDAP Foundation, info@OpenLDAP.org