Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist

[ando@sys-net.it]

Hallvard B Furuseth wrote:
> ando@sys-net.it wrote:
> 
> [about REP_ENTRY_MUSTRELEASE]
>> it is not
>> clear what happens when a callback chain is interrupted by
>> slap_null_cb() or similar, without getting to slap_send_search_entry().
>>   This seems to indicate that callbacks should always provide a last
>> resort means to release the resources they set; if read-only, by keeping
>> track of what they sent; if modifiable, by freeing the contents of
>> rs->sr_* if not NULL, setting it to NULL to prevent further cleanup.
> 
> That sounds cumbersome, I hope slapd could take care of that somehow.

Yes, it could: for example, by providing a helper like

	slap_entry2modifiable()

that takes s SlapReply, checks the flags and does nothing if the entry 
is already modifiable, or copies it if not, releasing the original one 
if REP_ENTRY_MUSTRELEASE was set.

> 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).  And the appropriate o_bd of a 
(non-modifiable) entry can be easily computed from the entry's DN.

> be->be_release() does receive entries that were not created by 'be' or
> at least not with be->be_fetch(), see openldap-devel thread 'slapd API'
> in mar 2008.

Might be a bug, but I'm not familiar with that code.

>> 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
>> is that REP_ENTRY_MODIFYABLE without REP_ENTRY_MUSTBEFREED implies that
>> the callback that set the former will take care of freeing the entry;
>> however, other callbacks may further modify it, so freeing temporary
>> data should probably be left to the final handler.
> 
> That's not my impression.  MODIFIABLE would be that other modules than
> the creator can modify the entry - but the creator might still be the
> one who will free it.  MUSTBEFREED is that the entry must be
> entry_free()ed - the creator will not do it (or not do it unless that
> flag is set).

They clearly mean different things; my point is that as soon as an entry 
is modifiable it is not read-only and thus is a temporary, and thus will 
eventually need to be freed.  So the point is who is actually going to 
free it.  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. 
  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...

> So, if I'm getting this right...
> 
> A backend must expect an entry to change or be freed if it sends the
> entry with REP_ENTRY_<MUSTRELEASE, MUSTBEFREED or MODIFIABLE>, or if
> it passes through a slap_callback.sc_response.

Right now, a backend expects the entry to change if sent with 
MODIFIABLE; to be released if sent with MUSTBERELEASED; to be freed if 
sent with MUSTBEFREED.  Modifications could occur to a MODIFIABLE entry 
when passing through a sc_callback().  Otherwise, if the entry is not 
MODIFIABLE, a copy will be created if the callback needs to modify the 
entry; after copying, the entry will be released if MUSTBERELEASED was 
set.  When a copy is created, the callback that creates the copy must 
also clear the MUSTBERELEASED flag.  Optionally, it may set the 
MUSTBEFREED flag if it doesn't intend to take care of cleaning the entry 
up after it's done.

> back-ldif does not: it uses e_name/e_nname _after_ sending with with
> REP_ENTRY_MODIFIABLE.

That should be considered a bug.

> Nor overlay retcode, it calls retcode_entry_response(,,,rs->sr_entry)
> which makes pointers into sr_entry without checking if those flags are
> set.  If I'm getting that code correctly.  I haven't tested.

I havent' checked, but that might be a bug as well.

> 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.
> translucent does not, even though it is a careful one which
> obeys both MUSTBEFREED and MUSTRELEASE.
> 
> 
> I'm not sure when the code must clear the entry_related flags.
> Some code which sets new entries seem to assume they are zero,
> but some code which sets sr_entry=NULL does not clear the flags.
> 
> There are sr_flags bits which are not about the entry, in particular
> REP_MATCHED_MUSTBEFREED and REP_MATCHED_MUSTBEFREED.  Looks like these
> flags can get lost when but some code sets sr_flags = <something>
> rather sr_flags |= <something> or sr_flags ~= ~<something>.

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.

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
---------------------------------------