Re: (ITS#5340) REP_ENTRY_MODIFIABLE bug in dynlist

[ando@sys-net.it]

h.b.furuseth@usit.uio.no wrote:
> h.b.furuseth@usit.uio.no 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.

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