create(), remove(): Before we write to the aliases.db file, we must

drop an exclusive advisory lock on it, so Postfix doesn't try to read
the file while we're updating it.  See the comment at the top of the
file for a discussion of why we have to do this so cruftily.

1 files changed, 47 insertions, 2 deletions

diff --git a/Mailman/MTA/Postfix.py b/Mailman/MTA/Postfix.py
index 7578435c5..db719a0e1 100644
--- a/Mailman/MTA/Postfix.py
+++ b/Mailman/MTA/Postfix.py
@@ -25,6 +25,7 @@ import time
 import dbhash
 import errno
 import pwd
+import fcntl
 from stat import *
 
 from Mailman import mm_cfg
@@ -38,6 +39,35 @@ LOCKFILE = os.path.join(mm_cfg.LOCK_DIR, 'creator')
 
 
 
+# Here's the deal with locking.  In order to assure that Postfix doesn't read
+# the file while we're writing updates, we should drop an exclusive advisory
+# lock on the file.  Ideally, we'd specify the `l' flag to dbhash.open() which
+# translates to passing the O_EXLOCK flag to the underlying bsddb open call --
+# on systems that support O_EXLOCK.
+#
+# Unfortunately, Linux is one of those systems that don't support O_EXLOCK.
+# To make matters worse, the default bsddb module in Python gives us no access
+# to the file descriptor of the database file, so we cannot dbhash.open(), dig
+# out the fd, then use fcntl.flock() to acquire the lock.  Bummer. :(
+#
+# Another approach would be to do a file dance to assure exclusivity.
+# I.e. when we add or remove list entries, we actually create a new .tmp file,
+# write all the entries to that file, and then rename() that file to
+# aliases.db.  The problem with /that/ approach though is that we can't get
+# the file ownership right on the tmp file. That's because the process adding
+# or removing entries may be Joe Shmoe who's a member of the `mailman' group,
+# or it may be a cgi process run as `nobody' or `apache'.  Or eventually, it
+# may be run via a mail program or any number of other processes.  Without a
+# setuid program we simply can't assure that the ownership of the tmp file
+# will be the same as the ownership of the original file, and /that/ we must
+# absolutely guarantee.  Postfix runs mail programs under the uid of the owner
+# of the aliases.db file.
+#
+# The best solution I can come up with involves opening the aliases.db file
+# with built-in open(), do the fd digout and flock(), then use dbhash.open()
+# to re-open the file through bsddb.  Blech.  If anybody has a better,
+# portable, solution, I'm all ears.
+
 def makelock():
     return LockFile.LockFile(LOCKFILE)
 
@@ -82,8 +112,13 @@ def create(mlist):
     # Acquire the global list database lock
     lock = makelock()
     lock.lock()
+    lockfp = None
     try:
-        # Crack open the dbhash file
+        # First, open the dbhash file using built-in open so we can acquire an
+        # exclusive lock on it.  See the discussion above for why we do it
+        # this way instead of specifying the `l' option to dbhash.open()
+        lockfp = open(DBFILE)
+        fcntl.flock(lockfp.fileno(), fcntl.LOCK_EX)
         db = dbhash.open(DBFILE, 'c')
         # Crack open the plain text file
         try:
@@ -101,6 +136,9 @@ def create(mlist):
         db.sync()
         fp.close()
     finally:
+        if lockfp:
+            fcntl.flock(lockfp.fileno(), fcntl.LOCK_UN)
+            lockfp.close()
         lock.unlock(unconditionally=1)
 
 
@@ -109,9 +147,13 @@ def remove(mlist):
     # Acquire the global list database lock
     lock = LockFile.LockFile(LOCKFILE)
     lock.lock()
+    lockfp = None
     try:
         listname = mlist.internal_name()
-        # Crack open the dbhash file, and delete all the entries
+        # Crack open the dbhash file, and delete all the entries.  See the
+        # discussion above for while we lock the aliases.db file this way.
+        lockfp = open(DBFILE)
+        fcntl.flock(lockfp.fileno(), fcntl.LOCK_EX)
         db = dbhash.open(DBFILE, 'c')
         for k, v in makealiases(listname):
             try:
@@ -168,6 +210,9 @@ def remove(mlist):
         outfp.close()
         os.rename(TEXTFILE+'.tmp', TEXTFILE)
     finally:
+        if lockfp:
+            fcntl.flock(lockfp.fileno(), fcntl.LOCK_UN)
+            lockfp.close()
         lock.unlock(unconditionally=1)