Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist

[ando@sys-net.it]

Hallvard B Furuseth wrote:
> Pierangelo Masarati writes:
>> Hallvard B Furuseth wrote:
>>> But I don't see how the be_release() code can work now.  It sounds like
>>> be->be_release() functions must check (how?) that the entry was created
>>> by 'be', and otherwise pass it on to the next overlay/backend or
>>> otherwise to entry_free().  Might involve mucking with op->o_bd and
>>> sr_entry->e_private, I suppose.  Except maybe I'm missing some existing
>>> magic since slapd doesn't regularly crash...
>> Yes, but that's trivial: e_private must be NULL for temporary entries,
>> and copying the entry loses it (no one is supposed to muck with it
>> expect the entry's creator).
> 
> OK...
> 
>> And the appropriate o_bd of a
>> (non-modifiable) entry can be easily computed from the entry's DN.
> 
> Hm?  Can only the "outermost" backend/overlay create non-modifiable
> entries?
> 
> I think any overlay can replace a non-modifiable rs->sr_entry value, as
> long as it restores it on the way out (unless mustbe<freed/released>).

That's why I'm not so happy with MUSTRELEASE.

>>>> Similarly, the existence of REP_ENTRY_MUSTBEFREED is not totally clear:
>>>> in principle as soon as REP_ENTRY_MODIFYABLE is set, it should imply
>>>> REP_ENTRY_MUSTBEFREED; the only difference in the semantics of the two
>>> That's not my impression. (...)
> 
> Some functions create entries on the stack an send them MODIFIABLE - but
> obviously without MUSTBEFREED.  They use entry_clean() instead.
> 
> Others set MODIFIABLE but not MUSTBEFREED on non-stack entries, and call
> entry_free after sending it.  Or forget to free it - back-perl/search.c
> if LDAP_SIZELIMIT_EXCEEDED.  (Not patching yet - don't have time to test
> it at the moment.)

OK, makes sense.  But perhaps creating entries on the stack could be 
deprecated now that entries are pooled and reused by calling 
entry_alloc() entry_free().  This is just food for thoughts, we could as 
well leave all those flags 'round, if we can streamline their handling.

>> (...) A copy might be created by an overlay after receiving a
>> read-only entry, but the same overlay might not actually perform the
>> copy if it receives the entry from another overlay that already copied
>> it, or from a proxy backend.  However, after the entry is copied the
>> overlay will have no means to determined who actually created the copy.
> 
> I don't think it matters who created a MUSTBEFREED copy:

right.

>>   This might be an issue depending on the order cleanup handlers are
>> called (didn't check what order they're called).  My point is that
>> temporary entries need to be freed at some point; who frees them should
>> not be relevant...
> 
> Right, someone must call entry_free() and clear rs->sr_entry (so it's
> not freed again), but entry_free() is not passed a backend parameter.
> 
> OTOH a MUSTBERELEASED entry must be released by the right
> backend/overlay, and it would be strange for such an entry to be
> MODIFIBALE.  Though I guess it could be - if the backend has no cache,
> and be_release just exists to clean up some data in e_private.
> 
>>> Others apparent problems (also not tested, I've just browsed the code):
>>> Overlays that obey and reset MUSTBEFREED or MUSTRELEASE, do not
>>> necessarily clear or set MODIFIABLE when setting a new entry.
>>> (...)
>> It seems to me that we should provide "smart" handlers to deal with
>> preparing sr_entry for modification, and to take care of cleaning things
>> up as appropriate.  Those helpers should then be consistently used in
>> the code.
> 
> Yup.  And add some aggressive asserts - at least #ifdef LDAP_DEVEL.
> Don't know when I'll have time for it though.

Making room in my todo list...

p.



Ing. Pierangelo Masarati
OpenLDAP Core Team

SysNet s.r.l.
via Dossi, 8 - 27100 Pavia - ITALIA
http://www.sys-net.it
---------------------------------------
Office:  +39 02 23998309
Mobile:  +39 333 4963172
Email:   pierangelo.masarati@sys-net.it
---------------------------------------