[Date Prev][Date Next]
Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist
Hallvard B Furuseth wrote:
> email@example.com 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
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
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
Ing. Pierangelo Masarati
OpenLDAP Core Team
via Dossi, 8 - 27100 Pavia - ITALIA
Office: +39 02 23998309
Mobile: +39 333 4963172