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