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, so it should be fine: if ( c->rvalue_nvals ) attr_merge(e, ct[i].ad, c->rvalue_vals, c->rvalue_nvals); else attr_merge_normalize(e, ct[i].ad, c->rvalue_vals, NULL);

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.

The attributes listed above tend to agree with that, but if they really were not being normalized before storage I'm surprised that the original assertion wasn't hit more often?