[Date Prev][Date Next]
Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist
> firstname.lastname@example.org writes:
>> overlays/dynlist.c lines 371-376 sets REP_ENTRY_MUSTBEFREED in rs->sr_flags
>> without duplicating the entry, if REP_ENTRY_MODIFIABLE was set.
>> Thus the entry gets freed twice. Breaks test044-dynlist with back-ldif.
> Actually obeying rs->sr_flags seems to be a more general problem - but I
> don't know what are bugs and what are my lacking understanding of the
> flags. I.e. when can an overlay change REP_ENTRY_MUSTBEFREED,
> REP_ENTRY_MUSTRELEASE? Usually backends/overlays that do not set these
> flags, don't seem to expect them to change either.
In general, callbacks are supposed to know about the existence of
rs->sr_flags and deal with them. In detail, callbacks should use
mechanisms that allow them to dispose of read-only resources without
counting on the fact that rs->sr_* fields will be persistent through the
callback chain. So, callbacks that need to modify data should check
whether that data is modifiable; if it's not, they should first copy it,
and set the rs->sr_flags as appropriate. The main reason for those
flags to exist is optimization, otherwise we'd always duplicate data and
dispose of it when done. So back-bdb/back-hdb, which are supposed to be
the high-end database implementations, will always pass read-only data
to the callback chain; however, if an overlay needs to muck with that
data, it will first copy it, and set the REP_ENTRY_MODIFYABLE to inform
other overlays that data can be freely modified without copying it
again. On its own, back-bdb/back-hdb will take care of releasing the
original entry without the need to use the value of rs->sr_entry; it'll
use its copy of the entry's pointer.
The existence of the REP_ENTRY_MUSTRELEASE value is not totally clear to
me; it seems to indicate that in some cases callbacks want to pass a
read-only entry without providing further cleanup code to release it; it
makes sense, since this simple operation can be done by
slap_send_search_entry() instead, if all that's required; however, this
means that any callback that replaces the entry must remember to release
it and must muck with rs->sr_flags accordingly. However, 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.
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.
Ing. Pierangelo Masarati
OpenLDAP Core Team
via Dossi, 8 - 27100 Pavia - ITALIA
Office: +39 02 23998309
Mobile: +39 333 4963172