[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: (ITS#5540) Normalization assertion in attr.c



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?

-- 
Thanks,
Sean Burford