Full_Name: Hallvard B Furuseth Version: OS: URL: Submission from: (NULL) (129.240.6.233) Submitted by: hallvard 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. This code snippet fixes it for test044, but it may be the wrong fix: e = rs->sr_entry; e_flags = rs->sr_flags; if ( !( e_flags & REP_ENTRY_MODIFIABLE ) ) { e = entry_dup( e ); if ( e_flags & REP_ENTRY_MUSTBEFREED ) entry_free( rs->sr_entry ); e_flags |= REP_ENTRY_MODIFIABLE | REP_ENTRY_MUSTBEFREED; } First, can something have references into the old sr_entry at this point? Second, there is also a REP_ENTRY_MUSTRELEASE flag which means to call be_entry_release_rw(), maybe that must be handled. Also overlays collect, pcache & valsort ignore REP_ENTRY_MUSTRELEASE. A common function to handle this might be useful - pass it a SlapResponse, BackendDB and flags to set and flags to unset, and let it free, release or dup what is needed.
moved from Incoming to Software Bugs
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. -- Hallvard
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 ---------------------------------------
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. 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... 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. > 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). 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. back-ldif does not: it uses e_name/e_nname _after_ sending with with REP_ENTRY_MODIFIABLE. 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. 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>. -- Hallvard
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 ---------------------------------------
Pierangelo Masarati writes: > Hallvard B Furuseth wrote: >> 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). OK... > And the appropriate o_bd of a > (non-modifiable) entry can be easily computed from the entry's DN. Hm? Can only the "outermost" backend/overlay create non-modifiable entries? I think any overlay can replace a non-modifiable rs->sr_entry value, as long as it restores it on the way out (unless mustbe<freed/released>). >>> 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 >> >> That's not my impression. (...) Some functions create entries on the stack an send them MODIFIABLE - but obviously without MUSTBEFREED. They use entry_clean() instead. Others set MODIFIABLE but not MUSTBEFREED on non-stack entries, and call entry_free after sending it. Or forget to free it - back-perl/search.c if LDAP_SIZELIMIT_EXCEEDED. (Not patching yet - don't have time to test it at the moment.) > (...) 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. I don't think it matters who created a MUSTBEFREED 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... Right, someone must call entry_free() and clear rs->sr_entry (so it's not freed again), but entry_free() is not passed a backend parameter. OTOH a MUSTBERELEASED entry must be released by the right backend/overlay, and it would be strange for such an entry to be MODIFIBALE. Though I guess it could be - if the backend has no cache, and be_release just exists to clean up some data in e_private. >> 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. >> (...) > > 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. Yup. And add some aggressive asserts - at least #ifdef LDAP_DEVEL. Don't know when I'll have time for it though. -- Hallvard
Hallvard B Furuseth wrote: > Pierangelo Masarati writes: >> Hallvard B Furuseth wrote: >>> 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). > > OK... > >> And the appropriate o_bd of a >> (non-modifiable) entry can be easily computed from the entry's DN. > > Hm? Can only the "outermost" backend/overlay create non-modifiable > entries? > > I think any overlay can replace a non-modifiable rs->sr_entry value, as > long as it restores it on the way out (unless mustbe<freed/released>). That's why I'm not so happy with MUSTRELEASE. >>>> 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 >>> That's not my impression. (...) > > Some functions create entries on the stack an send them MODIFIABLE - but > obviously without MUSTBEFREED. They use entry_clean() instead. > > Others set MODIFIABLE but not MUSTBEFREED on non-stack entries, and call > entry_free after sending it. Or forget to free it - back-perl/search.c > if LDAP_SIZELIMIT_EXCEEDED. (Not patching yet - don't have time to test > it at the moment.) OK, makes sense. But perhaps creating entries on the stack could be deprecated now that entries are pooled and reused by calling entry_alloc() entry_free(). This is just food for thoughts, we could as well leave all those flags 'round, if we can streamline their handling. >> (...) 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. > > I don't think it matters who created a MUSTBEFREED copy: right. >> 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... > > Right, someone must call entry_free() and clear rs->sr_entry (so it's > not freed again), but entry_free() is not passed a backend parameter. > > OTOH a MUSTBERELEASED entry must be released by the right > backend/overlay, and it would be strange for such an entry to be > MODIFIBALE. Though I guess it could be - if the backend has no cache, > and be_release just exists to clean up some data in e_private. > >>> 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. >>> (...) >> 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. > > Yup. And add some aggressive asserts - at least #ifdef LDAP_DEVEL. > Don't know when I'll have time for it though. Making room in my todo list... 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 ---------------------------------------
ando@sys-net.it writes: > That's why I'm not so happy with MUSTRELEASE. Seems to be required though. cvs log for slap.h revision 1.678. But it does look to me like SlapReply needs a BackendDB *e_owner member. I haven't observed buggy behavior related to that (or tried to provoke any), but it looks hairy for a backend with a be_release() function to make sure to intercept every entry from elsewhere and record previous owner info in e_private or callbacks. > OK, makes sense. But perhaps creating entries on the stack could be > deprecated now that entries are pooled and reused by calling > entry_alloc() entry_free(). This is just food for thoughts, we could as > well leave all those flags 'round, if we can streamline their handling. I think we might as well leave them. Since most are for saving memory, might even want to expand them eventually. (E.g. separate flags for _which_ parts of an entry are modifiable. Modifiable attribute list, DNs, values _in_ attribute list, etc.) For that matter, looking at entry_free/entry_alloc, I don't see how that pooling saves more than it costs. It saves 1 malloc for the Entry structure, but not the mallocs for the DN and attribute list. And it costs two global mutex lock/unlocks per entry, in entry_alloc() and entry_free(). An entry on the stack avoids both malloc and locks. Anyway, I guess one conclusion of all this is that the code really is as sloppy as it looked, there are no magic saving throws in there. (Except with finding the backend for be_release if I'm still missing something about that.) -- Hallvard
Below I'll summarize SlapReply.sr_flags as I now understand it after un-confusing myself a little, since this ITS is getting rather long. Mostly about how code which receives a filled SlapReply should use it, which I presume is primarily overlays, slap_callbacks, maybe back-relay. About discussion so far: The sr_flags are inconsistent about when which ones may or must be obeyed. E.g. REP_ENTRY_MUST<RELEASE/BEFREED> does not mean the entry will be released/freed, but REP_<MATCHED/REF> does in some functions. This needs to be normalized. We can state that overlays with a bi_entry_release_rw function must keep track of which module should release which entry, like glue does. At least for now. Never mind my worries about which overlay/backend should release an REP_ENTRY_MUSTRELEASE'd sr_entry. That doesn't matter with current OpenLDAP code. As far as I can see glue is the only overlay with a release function, and glue can dispatch on the entry DN. rs.sr_ref bugs, fixing now: - slap_send_ldap_result() forgets to free rs->sr_ref if it does not call send_ldap_response(). - send_ldap_response() can return with sr_ref set to a freed value. I'll make it return with sr_ref to NULL if REP_REF_MUSTBEFREED. ando@sys-net.it writes: > Similarly, the existence of REP_ENTRY_MUSTBEFREED is not totally clear: Looked a bit at cvs logs... Since you are the one who introduced MUSTBEFREED and MODIFIABLE (slap.h 1.503 for rwm) I think we can take it the intent of REP_ENTRY_MUSTBEFREED is indeed unclear/forgotten:-) Anyway, I think I've gotten the flags straight now: * REP_<ENTRY, MATCHED, REF>_MUST<RELEASE, BEFREED>: Overlays/callbacks may modify the rs->sr_<entry, text, ref> pointers. If they do, they must release/free the old value if and only if the corresponding MUST<RELEASE/BEFREED> flag is set. They should (re)set the relevant flags to describe the new value. Many overlays don't, or not thoroughly enough. - REP_<MATCHED, REF>_MUSTBEFREED: Callers setting these can depend on send_ldap_response() to free sr_<matched, ref>. That is, "send response/disconnect" calls. These flags do apparently not apply and should not be set when sending an entry or search continuation reference. Dunno why not REP_REF_MUSTBEFREED when sending continuation references. - REP_ENTRY_MUSTRELEASE and REP_ENTRY_MUSTBEFREED: Callers that set these flags when sending an entry, *allow* the callee to release/free rs->sr_entry. Callers cannot depend on it being release/freed, but some do anyway. It should simplify things to make the send functions always obey this flag, but I'm not sure how much cleanup that requires. There is partial code for that in slap_send_search_entry(), along with a FIXME note. Free MUSTBEFREED-entries with entry_free(), and MUSTRELEASE-entries with be_entry_release_r(). There is no "use be_entry_release_w()" flag, but no OpenLDAP code sends write-locked entries anyway. + REP_ENTRY_MUSTRELEASE: Created for ITS#3671 to ensure that network hangs don't delay cache entry unlocks: Modules should release their cache entry locks before sending anything. They can't do that with cached entries they send in rs->sr_entry, so instead they should set REP_ENTRY_MUSTRELEASE. Conversely, anything which does network I/O (and other potentially slooow ops I guess) should first obey this flag. Several overlays don't obey, nor some other code for sending an entry when it decides to send an error response instead. * REP_ENTRY_MODIFIABLE: Overlays and callbacks may modify the entry which rs->sr_entry points at if this flag is set. If they want to modify an incoming sr_entry when REP_ENTRY_MODIFIABLE is not set, they can use entry_dup(), release or free the old entry if sr_flags says so, put the new entry in sr_entry, and maybe set REP_ENTRY_MODIFIABLE | REP_ENTRY_MUSTBEFREED. Thus code which creates a temporary Entry struct and sends it as rs->sr_entry, should set REP_ENTRY_MODIFIABLE if the entry is of no further interest to it (and it has set no pointers into it, e.g. rs->sr_matched). That allows overlays that wish to modify the entry being sent, to use this entry instead of copying it. * REP_NO_<OPERATIONALS, ENTRYDN, SUBSCHEMA>: Set by the chain overlay to "tell the frontend not to add generated operational attributes". Only backend.c reacts to these flags. Dunno if overlays should reset these flags when replacing sr_entry with a "private" entry (with another DN) rather than a dup of the current one, or something. Some overlays set sr_flags to fresh values while others only update some bits in sr_flags, so the effect is likely somewhat ad-hoc. Leaving that worry to someone else. I've been fiddling with some functions to generalize this, but they seem to need a lot of flags or variants for different modes... Should maybe just commit some draft variants with warnings that they may change, and clean up later. After my changes don't cause slapd to crash, anyway:-) -- Hallvard
changed notes
h.b.furuseth@usit.uio.no wrote: > - REP_ENTRY_MUSTRELEASE and REP_ENTRY_MUSTBEFREED: > > Callers that set these flags when sending an entry, *allow* the > callee to release/free rs->sr_entry. > Callers cannot depend on it being release/freed, but some do anyway. > > It should simplify things to make the send functions always obey > this flag, but I'm not sure how much cleanup that requires. > There is partial code for that in slap_send_search_entry(), along > with a FIXME note. > > Free MUSTBEFREED-entries with entry_free(), and MUSTRELEASE-entries > with be_entry_release_r(). There is no "use be_entry_release_w()" > flag, but no OpenLDAP code sends write-locked entries anyway. I have the feeling that MUSTBEFREED and MUSTRELEASE should be consolidated into a single flag. be_entry_release_r() should simply call entry_free() if e->e_private is NULL. > + REP_ENTRY_MUSTRELEASE: > > Created for ITS#3671 to ensure that network hangs don't delay > cache entry unlocks: > > Modules should release their cache entry locks before sending > anything. They can't do that with cached entries they send in > rs->sr_entry, so instead they should set REP_ENTRY_MUSTRELEASE. > Conversely, anything which does network I/O (and other potentially > slooow ops I guess) should first obey this flag. > > Several overlays don't obey, nor some other code for sending an > entry when it decides to send an error response instead. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Howard Chu writes: > I have the feeling that MUSTBEFREED and MUSTRELEASE should be > consolidated into a single flag. be_entry_release_r() should simply > call entry_free() if e->e_private is NULL. Maybe. But consider translucent - it grabs incoming entries and remembers them. It can "steal" a MUSTBEFREED entry (though it doesn't now), but must duplicate a MUSTRELEASE entry so that it won't hold on to a lock in an underlying database. -- Hallvard
Belatead followup: This description of REP_ENTRY_MUSTRELEASE was still wrong. See ITS#5728: BDB didn't set the flag, ITS#3671 just gave the impression it would. Also code much take care to call overlay_entry_release_ov() instead of be_entry_release_r() when appropriate, as in ITS#5451. -- Hallvard
h.b.furuseth@usit.uio.no wrote: > ando@sys-net.it writes: >> OK, makes sense. But perhaps creating entries on the stack could be >> deprecated now that entries are pooled and reused by calling >> entry_alloc() entry_free(). This is just food for thoughts, we could as >> well leave all those flags 'round, if we can streamline their handling. > > I think we might as well leave them. Since most are for saving memory, > might even want to expand them eventually. (E.g. separate flags for > _which_ parts of an entry are modifiable. Modifiable attribute list, > DNs, values _in_ attribute list, etc.) > > For that matter, looking at entry_free/entry_alloc, I don't see how that > pooling saves more than it costs. It saves 1 malloc for the Entry > structure, but not the mallocs for the DN and attribute list. And it > costs two global mutex lock/unlocks per entry, in entry_alloc() and > entry_free(). An entry on the stack avoids both malloc and locks. Yes, using the stack is still preferable when it's doable. The point of entry_alloc() was to avoid heap fragmentation, not to generally save on mallocs. It might make sense to turn the entry lists into per-thread lists, to avoid locks in the default cases. But if you follow this path to completion, that turns into re-implementing tcmalloc ("tc" stands for "thread-caching" which is exactly what we would be duplicating...). As for the attribute list: attr_alloc() is also pre-allocated for the same reason. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
moved from Software Bugs to Development
I've been trying to factor this MUSTRELEASE/MUSTBEFREED stuff out to some macros. Easier said than done, I ended up with the macro monstrosity below. Do I put that in slap.h? Or a separate slap-response.h file? Generally the idea is that most code which modifies rs->sr_<entry/matched/ref/ctrls> (as opposed to initializing) should use these macros, which obey sr_flag and dispose of the old value if needed. There won't be that many callers actually, mostly overlays. There is overlay code which need to be rearranged a bit or which can't use this, and there is overlay code I don't understand at all and don't dare touch. Oh, and this seems to be wrong for how sr_ctrls are set up, which seems to be allocated differently some places. I'm just including that for reference for now, won't commit it yet in any case. Finally, code which aborts normal processing of some response and instead send an error often needs to be fixed - it often doesn't flush SlapReply data, and the data to be flushed and maybe freeed can be lost because it's overwritten. Generally I think code which modifies sr_type (or in some cases calls a function which will do so) should be responsible for flushing old SlapReply data first. #define REP_ENTRY_MUSTFLUSH (REP_ENTRY_MUSTBEFREED|REP_ENTRY_MUSTRELEASE) #define REP_ENTRY_MASK (REP_ENTRY_MODIFIABLE |REP_ENTRY_MUSTFLUSH) #define REP_MATCHED_MUSTFLUSH REP_MATCHED_MUSTBEFREED #define REP_REF_MUSTFLUSH REP_REF_MUSTBEFREED #define REP_CTRLS_MUSTFLUSH REP_CTRLS_MUSTBEFREED /* Macros updating freeable members of SlapReply, mostly for overlays. */ /* * Ensure rs->sr_entry is modifiable, by duplicating it if necessary. * Obey sr_flags. Set REP_ENTRY_<MODIFIABLE, and MUSTBEFREED if duplicated>. * Do <postmodActions> if the entry was replaced. */ #define RS_MODIFIABLE_ENTRY(op, rs, on, postmodActions) do { \ SlapReply *const rmeRS = (rs); \ if ( rmeRS->sr_flags & REP_ENTRY_MODIFIABLE ) break; \ RS_REPLACE_ENTRY( op, rmeRS, on, entry_dup( rmeRS->sr_entry ), 0 ); \ rmeRS->sr_flags |= REP_ENTRY_MODIFIABLE | REP_ENTRY_MUSTBEFREED; \ postmodActions; \ } while (0) /* Reset rs->sr_<member> if REP_<member>_MUSTBEFREED/MUSTRELEASE says so. */ #define RS_FLUSH_ENTRY(op, rs, on) RS_FLUSH(ENTRY, op, rs, on, 1, {}, {}) #define RS_FLUSH_CTRLS(op, rs) RS_FLUSH(CTRLS, op, rs, -, 1, {}, {}) #define RS_FLUSH_MATCHED( rs) RS_FLUSH(MATCHED, -, rs, -, 1, {}, {}) #define RS_FLUSH_REF( rs) RS_FLUSH(REF, -, rs, -, 1, {}, {}) /* Flush and set rs->sr_<member>. Set REP_<member>_MUSTBEFREED if <isDup>. */ #define RS_REPLACE_ENTRY(op,rs,on,v,isDup) RS_REPLACE(ENTRY,op,rs,on, v,isDup) #define RS_REPLACE_CTRLS(op,rs,v,isDup) RS_REPLACE(CTRLS, op,rs,-, v,isDup) #define RS_REPLACE_MATCHED( rs,v,isDup) RS_REPLACE(MATCHED, -,rs,-, v,isDup) #define RS_REPLACE_REF( rs,v,isDup) RS_REPLACE(REF, -,rs,-, v,isDup) /* Helpers for the above */ #define SR_ENTRY sr_entry #define SR_CTRLS sr_ctrls #define SR_MATCHED sr_matched #define SR_REF sr_ref typedef Entry *SR_ENTRY_t; typedef LDAPControl **SR_CTRLS_t; typedef const char *SR_MATCHED_t; typedef BerVarray SR_REF_t; #define RS_FREE_ENTRY(op, rs, on)\ if ((rs)->sr_flags & REP_ENTRY_MUSTBEFREED) { \ assert (!(rs)->sr_flags & REP_ENTRY_MUSTRELEASE); \ entry_free((rs)->sr_entry); \ } \ else if (on == NULL) be_entry_release_rw(op, (rs)->sr_entry, 0);\ else overlay_entry_release_ov(op, (rs)->sr_entry, 0, on) #define RS_FREE_CTRLS(op, rs, on) slap_free_ctrls( op, (rs)->sr_ctrls ) #define RS_FREE_MATCHED(op, rs, on) ch_free( (char *) (rs)->sr_matched ) #define RS_FREE_REF(op, rs, on) ber_bvarray_free( (rs)->sr_ref ) #define RS_FLUSH(MEMBER, op, rs, on, reset, preActions, postActions) do { \ SlapReply *const rfRS = (rs); \ preActions; \ if ( rfRS->sr_flags & REP_##MEMBER##_MUSTFLUSH ) { \ if ( rfRS->SR_##MEMBER ) { \ RS_FREE_##MEMBER( op, rfRS, on ); \ if ( reset ) rfRS->SR_##MEMBER = NULL; \ } \ } \ rfRS->sr_flags &= ~REP_##MEMBER##_MASK; \ postActions; \ } while (0) #define RS_REPLACE(MEMBER, op, rs, on, val, isDup) \ RS_FLUSH( MEMBER, op, rs, on, 0, \ SR_##MEMBER##_t const rfNewval = (val), \ if ( isDup ) rfRS->sr_flags |= REP_##MEMBER##_MUSTBEFREED; \ rfRS->SR_##MEMBER = rfNewval ) /* Warn if something re#defines the following, which would break RS_REPLACE */ #define ENTRY ENTRY #define CTRLS CTRLS #define MATCHED MATCHED #define REF REF
I've been trying to do too much at once in this ITS. My macros can be simplified to at least use functions for the exposed API. Since we now have commits related to this (ITS#6423), there's no reason not to use a common function for the new commits, instead of trying to get it right each time. I've committed a variant of this, so far unused: slap.h REP_ENTRY_MUSTFLUSH, result.c rs_replace_entry(), rs_ensure_entry_modifiable() When an overlay can wants to update rs->sr_entry it can use these. Exception: Some code may want to temporarily replace the entry and later restore it. That should be by a push/pop of sr_entry and (sr_flags & REP_ENTRY_MASK) together. That is, push: <save rs->sr_entry and rs->rs_flags>; rs->sr_flags &= ~REP_ENTRY_MASK; rs->sr_entry = new entry; rs->sr_flags |= new entryflags, if any; pop: rs_replace_entry(op, rs, on, <old entry>); rs->sr_flags |= (<saved flags> & REP_ENTRY_MASK); Maybe this is what dynlist.c:dynlist_prepare_entry() wants to do? I don't see why else it waits to the end of the function to set rs->sr_flags/sr_entry to the _new_ flags and entry. Otherwise it should use rs_ensure_entry_modifiable(). Currently it's still wrong: Discards the old entry without obeying sr_flags. I don't know what overlay to pass to entry_modifiable though. -- Hallvard
I wrote: > Otherwise it should use rs_ensure_entry_modifiable(). Currently > it's still wrong: Discards the old entry without obeying sr_flags. Unless dynlist_sc_save_entry() handles it, I haven't yet tried to figure out the state when it is called. But it looks wrong: it saves and clears rs->sr_entry without saving/clearing (rs->sr_flags & REP_ENTRY_MASK). Here is a draft for what rs_ensure_entry_modifiable() usage is supposed to look like, though I haven't tested it well and I'm going home now. I'm unsure of the translucent code. rwm looks a bit weird, but that's mostly to assert that some functions that receive rs won't modify it. I haven't touched dynlist, don't know what entry ownership is there. http://folk.uio.no/hbf/OpenLDAP/slapreply-draft.diff -- Hallvard
[Copying some discussion from IRC, and adding some more] I'm a bit nervous about doing more than minimal fixes to this until we do something major, because it's so broken. Fix one bit and maybe something will pop up another place - i.e. a regression. Also following existing practice can mean fix one bug into a different bug, which Ando recently did for ITS#6423. (dynlist resets MUSTRELEASE but doesn't obey it, rwm works around a similar bug in rwm or dynlist.) Though he got other fixes right. Anyway: To repeat, the main problems with the current code: * Code does not always clean up properly, leaving e.g. flags behind. * Code which does clean up sometimes does it halfway, e.g. it resets MUSTBEFREED/MUSTRELEASE but not MODIFIABLE. So, no wonder there are unwanted MODIFIABLE flags hanging about (ITS#6423). Or it dup()s an entry but does not release the old one. * op_*() generally expects SlapReply to be preinitialized. That worked before overlays, but now modules (backends, overlays) inherit values that the previous module did not clean up during an operation. Howard suggested that we should restore that - code which puts something into a SlapReply should ensure it's taken out again. I've started on that in the enclosed patch, but on second thought I think that's not the right fix because it's the most fragile solution: Practically every module which depends on it can break if some other module gets it wrong. OTOH, modules can't just reset SlapReply since some of it is input values to op_*(): - Controls can be set in backend_check_restrictions() before the call to op_*(). - dynlist.c sets r.sr_attr_flags before calling o.o_bd->be_search(), I haven't looked at what effect that has. I'm thinking modules should instead initialize SlapReply.sr_u and maybe some sr_flags themselves instead of depending on preinit, but I don't know when I'll get back to doing something about that. Xmas coming... So anyway, here is a quick patch which actually tries to be rather conservative, sans a bunch of RS_ASSERT() lines which can be safely deleted in real code. It only addresses sr_entry and sporadically attrs, not sr_refs/ctrls. It passes make test for me. Please review. Where updates to flags are broken and needs fixing, I change to use the new function rs_ensure_entry_modifiable() etc, but I leave correct updates alone - as well as some updates where I don't know which overlay should be passed to these function. Description of changes: All over the place: Clear MUSTBEFREED/MUSTRELEASE by clearing all of REP_ENTRY_MASK, not just the particular flag. Reset attrs, entry, flags a number of places. (Not very consistently, sorry.) Lots of RS_ASSERT()s for debugging. Only defined when LDAP_DEVEL. proto-slap.h, result.c: Move RS_ASSERT(), add rs_flush_entry(). result.c: MODIFIABLE is independent of MUSTBEFREED - the modifiable entry can e.g. be on the stack. result.c, extended.c: When changing sr->sr_type to extended/sasl, flush sr_un (usually the entry) first, since another member of the union will be used. For this reason, updates to sr_type are "suspicious" code to be avoided, so some code elsewhere is changed to initialize it to REP_RESULT rather than setting it to that after initialization. bconfig.c, servers/slapd/syncrepl.c: Avoid reusing SlapReply a number of places where that's easy. slap.h: Cast the REP_... flags to slap_mask_t, so ~REP_... will work right. back-bdb/search.c: Delay setting rs->sr_entry until we know it'll be used. (IIRC made a difference around a paged results response.) back-shell/result.c, back-sock/result.c: Set MUSTBEFREED before sending, obey it afterwards. overlays/constraint.c, overlays/pcache.c: Drop nulling of fields that were just nulled by initialization. overlays/dynlist.c: When "stealing" an entry from SlapReply, grab and reset the flags that go with it, so it can be properly freed/released. Restore the flags and entry later, then obey the flags (release/free). Do this unconditionally, it happens only once per SlapReply anyway. overlays/pcache.c: Obey and reset entry flags properly. Remove unused SlapReply parameters to some functions - did this for readibility while trying to figure out what was going on. overlays/sssvlv.c: Since setting MUSTRELEASE before sending, obey it afterwards. overlays/translucent.c: Reset flags properly too when releasing/freeing entry. Streamline translucent_search() loop to clarify ownership of entry. overlays/rwm.c, servers/slapd/slapi/slapi_pblock.c: Obey flags properly.
Forgot to enclose the URL. http://folk.uio.no/hbf/OpenLDAP/slapreply-draft2.diff
> Forgot to enclose the URL. > > http://folk.uio.no/hbf/OpenLDAP/slapreply-draft2.diff At a first glance, your patch makes sense. I see your concern, this approach is definitely fragile. However, I see two points: - the code is not just modified by everyone, so we can count on paying relative attention to what is done, as soon as there are clear guidelines - we should probably produce some indications about how code needs to be used. I'm not thinking about a comprehensive programming guide, but some clear guidelines about writing safe code in critical areas like this would be of help. In cases like this, I'd really prefer to be able to exploit OOP and data abstraction as in C++. p.
changed notes changed state Open to Closed
Some fixes in RE24. Moved the rest to ITS#6758. Long discussion, skip to message #8. See also ITS#6423.