openldap-bugs September 2012

openldap-bugs@openldap.org

29 participants
107 discussions

Re: (ITS#7364) mdb: clean up POSIX semaphores on environment close.
by h.b.furuseth＠usit.uio.no 01 Sep '12

01 Sep '12

Reopening this. This is worse with a database with is intended to be used by different users (A and B): A opens the DB and creates the semaphores with e.g. mode 0660. B opens it, A closes, B closes - and fails sem_unlink() which only A and root can do. Next, B (or C) fails mdb_env_open() because sem_unlink() fails again. The work-around I can think of is a "multi-uid" mode which instead resets the semaphore with sem_post() if sem_getvalue() returns 0. I don't know how ugly that is considered to be. Could ask comp.programming.unix, or check what Berkeley DB does. This mode should use mode 0666 for the semaphores (temporarily setting umask 0, yuck), or it should not sem_unlink() since next user may create the semaphores with a group which gives the wrong users access. Or root may give it root-only access, as in the original report. If not unlinking, we need a special "remove database + semaphores" API call. Or use some other sync primitive, like file locks. The Linux manual says these work over NFS though, which sounds like they must be rather slow. flock() does not, but this time mdb would need to seek first. I don't know which sync variants BSD offers. Other matters with the current implementation - I'll patch these: mdb_env_excl_lock() need not retry getting a non-exclusive lock when closing. mdb_env_close() can pass *excl = -1 to tell it not to. mdb_env_setup_locks() can sem_unlink both semaphores before doing anything else, so that reopening a database as root will clean up. Drop the error checks of sem_unlink (so both get called), instead use O_EXCL in sem_open(,O_CREAT,,). Unless I'm missing something, the error checks just work like an emulation of O_EXCL anyway.

1 0

Re: (ITS#7363) libmdb should use POSIX semaphores on non-apple BSD systems too.
by h.b.furuseth＠usit.uio.no 01 Sep '12

01 Sep '12

Chris Mikkelson writes: > The *BSD systems define PTHREAD_PROCESS_SHARED but do > not implement sharable pthread mutexes. The flag name > is part of the API, but implementation of the behavior > it requests is optional. Yes, sorry about that. How about something like a "__BSD" preprocessor symbol to replace "BSD"? There surely is one. It'll start with an underscore followed by either another underscore or an uppercase letter. -- Hallvard

1 0

Re: (ITS#7377) Poor libmdb error handling
by Hallvard B Furuseth 01 Sep '12

01 Sep '12

Howard Chu wrote: >h.b.furuseth(a)usit.uio.no wrote: >> Maybe add a flag to request error paranoia, like setting >> PTHREAD_MUTEX_ERRORCHECK. Trying to catch if a current mdb >> process has lost is locks. (The fcntl F_SETLK is lost if the >> process closes another file descriptor to the same file, >> e.g. after some other module in the process opened and closed the >> mdb lock file.) Catching a change to the pid from getpid(). > > That's really not interesting to me. No other module has any business > opening and reading our lock file. I skipped a step there. The other module could also be using mdb. "other module" = "a program component which the module using this MDB_env does not know about". They may even be linking different libmdb.so's, so the 'libmdb's can't notify each other via static vars. There are a few other use cases like a monolith program which also can backup a directory - which the user could point at the mdb dir. Anyway, a CAVEATS section in the doc is the obvious fix. > We haven't bothered with this for alock either, and > it hasn't been a problem. Note that this ITS is about libmdb, not back-mdb. >> The fork() restriction needs to be documented, as does "don't open >> or close the same database twice in the same process". > > Go ahead. Hmm. I suggested a too pessimistic caveats section once, I can dig that up and try again.

1 0

Re: (ITS#7377) Poor libmdb error handling
by hyc＠symas.com 01 Sep '12

01 Sep '12

h.b.furuseth(a)usit.uio.no wrote: > Full_Name: Hallvard B Furuseth > Version: 2.4.32 > OS: Linux x86_64 > URL: > Submission from: (NULL) (193.69.163.163) > Submitted by: hallvard > > > libmdb omits necessary checks for system call errors, and does > not always clean up after the errors it does detect. > > Also mdb could catch more user errors - including "cannot happen" > mutex errors from mutex lock/unlock, since some threading user > errors are be non-obvious from the doc. > > At least sem_wait() needs an EINTR loop. POSIX says "The > sem_wait() function is interruptible by the delivery of a signal." > The Linux manpage adds that a signal handler always interrupts it > regardless of sigaction SA_RESTART. I don't know about BSD. > > Some error checks first need updates to the Windows emulation, or > will look cleaner that way. E.g. pthread_key_create(). > > I'll fix mdb_env_open()'s error cleanup, at least. > (Hyc, that moves cleanup out of mdb_env_setup_locks(). Any > preference for using ~25 exit points 'return <ErrCode() or rc>' > vs. ~25 'goto <failure label>' which does the same?) In general I prefer that functions have only one exit point, so multiple gotos is preferable. > Maybe add a flag to request error paranoia, like setting > PTHREAD_MUTEX_ERRORCHECK. Trying to catch if a current mdb > process has lost is locks. (The fcntl F_SETLK is lost if the > process closes another file descriptor to the same file, > e.g. after some other module in the process opened and closed the > mdb lock file.) Catching a change to the pid from getpid(). That's really not interesting to me. No other module has any business opening and reading our lock file. We haven't bothered with this for alock either, and it hasn't been a problem. Change of pid... not worth bothering. Just add a note to the docs. > Regarding the pid: Repeating getpid() - in mdb_txn_renew0() and > mdb_env_close() - is useless since mdb cannot survive fork() > anyway. fork drops fcntl locks. Might as well store the pid (or > just some unique "environment id") in MDB_env and use that. Agreed. I had a patch here that does just that, seems it got lost in some other edits. > OTOH > some strategically placed getpid()s can then catch the user error > of having forked, and set e.g. MDB_FATAL_ERROR. Not really interesting. > The fork() restriction needs to be documented, as does "don't open > or close the same database twice in the same process". Go ahead. > mdb_env_open() should catch being called twice. Except, should > it try to allow a 2nd call if 1st mdb_env_open() failed? It can > easily do that if 1st attempt failed before reading params from > the DB files, but fail if the 1st open got that far. Called twice on the same env handle? Again, not interesting. As a rule, we should of course check for failure return codes from normal operation. We should not add extensive checks for programmer error/misuse of API. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/

1 0

Re: (ITS#7377) Poor libmdb error handling
by openldap-its＠OpenLDAP.org 01 Sep '12

01 Sep '12

I messed up last reply headers somehow. Trying again. I wrote: > Also mdb could catch more user errors - including "cannot happen" > mutex errors from mutex lock/unlock, since some threading user > errors are be non-obvious from the doc. That is, "cannot happen" when there is no user error. > The fork() restriction needs to be documented, as does "don't > open or close the same database twice in the same process". Correction, "don't have the same database open twice at the same time in the same process".

1 0

Re: (ITS#7377) Poor libmdb error handling
by openldap-its＠OpenLDAP.org 01 Sep '12

01 Sep '12

I wrote: > Also mdb could catch more user errors - including "cannot happen" > mutex errors from mutex lock/unlock, since some threading user > errors are be non-obvious from the doc. That is, "cannot happen" when there is no user error. > The fork() restriction needs to be documented, as does "don't > open or close the same database twice in the same process". Correction, "don't have the same database open twice at the same time in the same process".

1 0

(ITS#7377) Poor libmdb error handling
by h.b.furuseth＠usit.uio.no 01 Sep '12

01 Sep '12

Full_Name: Hallvard B Furuseth Version: 2.4.32 OS: Linux x86_64 URL: Submission from: (NULL) (193.69.163.163) Submitted by: hallvard libmdb omits necessary checks for system call errors, and does not always clean up after the errors it does detect. Also mdb could catch more user errors - including "cannot happen" mutex errors from mutex lock/unlock, since some threading user errors are be non-obvious from the doc. At least sem_wait() needs an EINTR loop. POSIX says "The sem_wait() function is interruptible by the delivery of a signal." The Linux manpage adds that a signal handler always interrupts it regardless of sigaction SA_RESTART. I don't know about BSD. Some error checks first need updates to the Windows emulation, or will look cleaner that way. E.g. pthread_key_create(). I'll fix mdb_env_open()'s error cleanup, at least. (Hyc, that moves cleanup out of mdb_env_setup_locks(). Any preference for using ~25 exit points 'return <ErrCode() or rc>' vs. ~25 'goto <failure label>' which does the same?) Maybe add a flag to request error paranoia, like setting PTHREAD_MUTEX_ERRORCHECK. Trying to catch if a current mdb process has lost is locks. (The fcntl F_SETLK is lost if the process closes another file descriptor to the same file, e.g. after some other module in the process opened and closed the mdb lock file.) Catching a change to the pid from getpid(). Regarding the pid: Repeating getpid() - in mdb_txn_renew0() and mdb_env_close() - is useless since mdb cannot survive fork() anyway. fork drops fcntl locks. Might as well store the pid (or just some unique "environment id") in MDB_env and use that. OTOH some strategically placed getpid()s can then catch the user error of having forked, and set e.g. MDB_FATAL_ERROR. The fork() restriction needs to be documented, as does "don't open or close the same database twice in the same process". mdb_env_open() should catch being called twice. Except, should it try to allow a 2nd call if 1st mdb_env_open() failed? It can easily do that if 1st attempt failed before reading params from the DB files, but fail if the 1st open got that far.

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs September 2012