Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: http://folk.uio.no/hbf/OpenLDAP/bdb0727.diff Submission from: (NULL) (129.240.186.42) Submitted by: hallvard back-bdb/search.c:bdb_search() gets a LOCK_ID() if !(opinfo && opinfo->boi_txn), but frees it if !opinfo. A fix is to free it if !ltid instead; that's set iff (opinfo && opinfo->boi_txn). Also this is not done before all return statements. I'll fix (patch attached) if someone confirms; bdb is a bit too magical for me to meddle with locks like that without asking:-) I would note that I generally find lock/unlock-code easier to read with just one unlock per lock, i.e. using goto <unlock; return> instead of multiple {unlock; return}. Might have avoided the bug with not unlocking before each return too. That's a bigger change to 6 files, so I'm leaving that alone unless someone says yes.
h.b.furuseth@usit.uio.no wrote: > Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: > http://folk.uio.no/hbf/OpenLDAP/bdb0727.diff Submission from: (NULL) > (129.240.186.42) Submitted by: hallvard > > > back-bdb/search.c:bdb_search() gets a LOCK_ID() if !(opinfo && > opinfo->boi_txn), but frees it if !opinfo. A fix is to free it if > !ltid instead; that's set iff (opinfo && opinfo->boi_txn). > > Also this is not done before all return statements. > > I'll fix (patch attached) if someone confirms; bdb is a bit too > magical for me to meddle with locks like that without asking:-) That patch looks fine. > I would note that I generally find lock/unlock-code easier to read > with just one unlock per lock, i.e. using goto <unlock; return> > instead of multiple {unlock; return}. I agree. The original code mostly does that; these two cases appear to have been added more recently and did not follow the established pattern. > Might have avoided the bug with not unlocking before each return too. > That's a bigger change to 6 files, so I'm leaving that alone unless > someone says yes. I'll note that the LOCK_ID_FREE() invocations here are actually no-ops, so their omission won't actually cause any malfunction in a default compile. It's cruft left over from when we didn't use long-lived per-thread locker IDs. Since it appears we've overcome all the issues with long-lived lockers and don't need to consider the possibility of running with #undef BDB_REUSE_LOCKERS it may be a better idea to just unifdef this stuff and remove it. -- -- Howard Chu Chief Architect, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc OpenLDAP Core Team http://www.openldap.org/project/
changed notes changed state Open to Closed
moved from Incoming to Archive.Incoming
code in question is no-op'd