[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
Re: (ITS#5540) Normalization assertion in attr.c
------=_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 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?
+ 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
> ) ) ) );
> }
>
> if ( vals != NULL ) {
>
> --------------050600010007030200030106--
>
>
>
--
Thanks,
Sean Burford
------=_Part_7327_2415938.1212167698754
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Hi,<br><br><div class="gmail_quote">On Fri, May 30, 2008 at 2:40 AM, <<a href="mailto:hyc@symas.com";>hyc@symas.com</a>> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
This is a multi-part message in MIME format.<br>
--------------050600010007030200030106<br>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed<br>
Content-Transfer-Encoding: 7bit<br>
<div class="Ih2E3d"><br>
<a href="mailto:hyc@symas.com";>hyc@symas.com</a> wrote:<br>
> We'll probably need to discuss on the -devel list what steps to take from here.<br>
><br>
</div>I wrote the attached patch for the original issue. Unfortunately it asserted<br>
right away:<br>
<br>
[Switching to Thread 1090525504 (LWP 23577)]<br>
0x00002b55e738d535 in raise () from /lib64/libc.so.6<br>
(gdb) bt<br>
#0 0x00002b55e738d535 in raise () from /lib64/libc.so.6<br>
#1 0x00002b55e738e990 in abort () from /lib64/libc.so.6<br>
#2 0x00002b55e7386c16 in __assert_fail () from /lib64/libc.so.6<br>
#3 0x00000000004410e5 in attr_valadd (a=0xb866c8, vals=0x41000670, nvals=0x0,<br>
nn=1) at ../../../r24/servers/slapd/attr.c:394<br>
#4 0x0000000000441a06 in attr_merge_one (e=0xb72c08, desc=0x8b3780,<br>
val=0x41000670, nval=0x0)<br>
at ../../../r24/servers/slapd/attr.c:599<br>
#5 0x000000000043ea0f in slap_add_opattrs (op=0x965ec0, text=<value optimized<br>
out>, textbuf=<value optimized out>,<br>
textlen=<value optimized out>, manage_ctxcsn=<value optimized out>) at<br>
../../../r24/servers/slapd/add.c:664<br>
#6 0x00000000004d9c42 in hdb_add (op=0x965ec0, rs=0x41000ca0) at add.c:108<br>
#7 0x000000000043f1a6 in fe_op_add (op=0x965ec0, rs=0x41000ca0) at<br>
../../../r24/servers/slapd/add.c:334<br>
#8 0x000000000043fa12 in do_add (op=0x965ec0, rs=0x41000ca0) at<br>
../../../r24/servers/slapd/add.c:194<br>
#9 0x00000000004383a7 in connection_operation (ctx=0x41000de0,<br>
arg_v=0x965ec0) at ../../../r24/servers/slapd/connection.c:1084<br>
#10 0x00000000004388cf in connection_read_thread (ctx=0x41000de0, argv=0xc) at<br>
../../../r24/servers/slapd/connection.c:1211<br>
#11 0x000000000054b8ca in ldap_int_thread_pool_wrapper (xpool=0x8bf070) at<br>
../../../r24/libraries/libldap_r/tpool.c:663<br>
#12 0x00002b55e655e09e in start_thread () from /lib64/libpthread.so.0<br>
#13 0x00002b55e741e4cd in clone () from /lib64/libc.so.6<br>
#14 0x0000000000000000 in ?? ()<br>
<br>
It was adding the createTimestamp attribute, without providing normalized<br>
values. slap_add_opattrs was written before the generalizedTimeNormalize<br>
function was written... I suspect there will be a fair number of cases that<br>
need to be cleaned up. I don't have time at the moment to chase them all down.<br>
Anyone else want to jump in here? If not, we may have to push this back a bit.<br>
Note that this patch will probably require a fair number of databases to be<br>
reloaded.<br>
<div class="Ih2E3d"><br>
--<br>
-- Howard Chu<br>
CTO, Symas Corp. <a href="http://www.symas.com"; target="_blank">http://www.symas.com</a><br>
Director, Highland Sun <a href="http://highlandsun.com/hyc/"; target="_blank">http://highlandsun.com/hyc/</a><br>
Chief Architect, OpenLDAP <a href="http://www.openldap.org/project/"; target="_blank">http://www.openldap.org/project/</a><br>
<br>
</div>--------------050600010007030200030106<br>
Content-Type: text/plain;<br>
name="dif.txt"<br>
Content-Transfer-Encoding: 7bit<br>
Content-Disposition: inline;<br>
filename="dif.txt"<br>
<br>
Index: attr.c<br>
===================================================================<br>
RCS file: /repo/OpenLDAP/pkg/ldap/servers/slapd/attr.c,v<br>
retrieving revision <a href="http://1.112.2.7"; target="_blank">1.112.2.7</a><br>
diff -u -r1.112.2.7 attr.c<br>
--- attr.c 11 Feb 2008 23:26:43 -0000 <a href="http://1.112.2.7"; target="_blank">1.112.2.7</a><br>
+++ attr.c 30 May 2008 09:33:43 -0000<br>
@@ -366,8 +366,39 @@<br>
BerVarray nvals,<br>
int nn )<br>
{<br>
- int i;<br>
BerVarray v2;<br>
+ int i;<br>
+<br>
+ {<br>
+ /* FIXME: if the schema has been edited, and an equality matching rule<br>
+ * has been added or removed from the attribute definition, the database<br>
+ * values may no longer be in sync with our expectations. Currently this<br>
+ * means the DB must be reloaded.<br>
+ */<br>
+ int have_norm, have_nval, new_nval;<br>
+ have_norm = a->a_desc->ad_type->sat_equality->smr_normalize != NULL;</blockquote><div><br>Not all attributes have an equality matching rule. This causes your patch to
segfault since sat_equality is NULL:<br>
<blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex; font-family: courier new,monospace;" class="gmail_quote">have_norm = a->a_desc->ad_type->sat_equality->smr_normalize != NULL;<br>
</blockquote>
<br>This seems to be a better way to generate
have_norm:<br><blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote"><span style="font-family: courier new,monospace;">have_norm = a->a_desc->ad_type->sat_equality != NULL &&</span><br style="font-family: courier new,monospace;">
<span style="font-family: courier new,monospace;"> a->a_desc->ad_type->sat_equality->smr_normalize != NULL;</span><br></blockquote><br>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.<br>
<br>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?<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ have_nval = a->a_nvals != a->a_vals;<br>
+ new_nval = nvals != NULL;<br>
+<br>
+ /* this check is only relevant if any values already exist */<br>
+ if ( a->a_vals != NULL && have_norm != have_nval ) {<br>
+ Debug(LDAP_DEBUG_ANY,<br>
+ "attr_valadd: database inconsistent with schema definition of %s, reload the DB\n",<br>
+ a->a_desc->ad_cname.bv_val, 0, 0 );<br>
+ return LDAP_OTHER;<br>
+ /* no need to compare have_nval with new_nval; by transitivity they will match if<br>
+ * the above check and the following check succeed.<br>
+ */<br>
+ }<br>
+<br>
+ assert( have_norm == new_nval );<br>
+ if ( have_norm != new_nval ) {<br>
+ Debug(LDAP_DEBUG_ANY,<br>
+ "attr_valadd: new values of %s not properly normalized (should never happen!)\n",<br>
+ a->a_desc->ad_cname.bv_val, 0, 0 );<br>
+ return LDAP_OTHER;<br>
+ }<br>
+ }<br>
<br>
v2 = (BerVarray) SLAP_REALLOC( (char *) a->a_vals,<br>
(a->a_numvals + nn + 1) * sizeof(struct berval) );<br>
@@ -469,15 +500,6 @@<br>
<br>
if ( *a == NULL ) {<br>
*a = attr_alloc( desc );<br>
- } else {<br>
- /*<br>
- * FIXME: if the attribute already exists, the presence<br>
- * of nvals and the value of (*a)->a_nvals must be consistent<br>
- */<br>
- assert( ( nvals == NULL && (*a)->a_nvals == (*a)->a_vals )<br>
- || ( nvals != NULL && (<br>
- ( (*a)->a_vals == NULL && (*a)->a_nvals == NULL )<br>
- || ( (*a)->a_nvals != (*a)->a_vals ) ) ) );<br>
}<br>
<br>
if ( vals != NULL ) {<br>
<br>
--------------050600010007030200030106--<br>
<br>
<br>
</blockquote></div><br><br clear="all"><br>-- <br>Thanks,<br>Sean Burford
------=_Part_7327_2415938.1212167698754--