Last Comment Bug 384774 - Mailnews:movemail. Fails in the increasingly common situation of no write privs in mail spool dir
: Mailnews:movemail. Fails in the increasingly common situation of no write pri...
Product: MailNews Core
Classification: Components
Component: Movemail (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: Thunderbird 3.0b4
Assigned To: Joshua Cranmer [:jcranmer]
: 56671 239013 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2007-06-17 03:59 PDT by Sune Mølgaard
Modified: 2010-09-18 03:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch, version 1 (4.85 KB, patch)
2009-06-28 14:19 PDT, Joshua Cranmer [:jcranmer]
mnyromyr: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description Sune Mølgaard 2007-06-17 03:59:59 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv: Gecko/20070509 SeaMonkey/1.1.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv: Gecko/20070509 SeaMonkey/1.1.2

I have seen a lot of reports about this on google. Furthermore, postfix is known to reset permissions on the spool dir.

Current mailutils movemail reverts to kernel locking in this event, so fixing will fix this too.

Alternatively, kernel locking could be implemented in the internal movemail.

Reproducible: Always

Steps to Reproduce:
1. Verify no user writability on /var/mail/ or equivalent.
2. Set up a movemail account
3. Try to receive mails
Actual Results:  
Error dialog

Expected Results:  
Mails fetched

Known to be a problem on at least Ubuntu (several versions). Am bugging them too (
Comment 1 Wayne Mery (:wsmwk, NI for questions) 2008-07-14 19:25:01 PDT
Sune says "[Ubuntu] said to bug the mozilla dev team..."
Comment 2 Joshua Cranmer [:jcranmer] 2008-08-16 11:20:07 PDT
Bug is confirmed on Linux Trunk.

This also seems to be a very, very cross-distro problem.
Comment 3 Joshua Cranmer [:jcranmer] 2008-08-16 11:36:17 PDT
FWIW, GNU's movemail locking first tries to do the "dotlock" (what TB does now), and only does kernel locking if the access isn't permitted.

The kernel locking is done with file descriptors holding a write lock.
Comment 4 Joshua Cranmer [:jcranmer] 2008-08-16 11:48:34 PDT
*** Bug 239013 has been marked as a duplicate of this bug. ***
Comment 5 Joshua Cranmer [:jcranmer] 2008-08-16 11:53:05 PDT
*** Bug 56671 has been marked as a duplicate of this bug. ***
Comment 6 Joshua Cranmer [:jcranmer] 2009-06-28 14:19:13 PDT
Created attachment 385658 [details] [diff] [review]
Patch, version 1

This patch fixes the bug in a moderately cross-platform way, and works on my Debian setup both with 755 and 777 permissions on /var/spool/mail.

Looking at the source for PR_TLockFile, it seems to lock in the same way as GNU mailutils does, so it should handle that case nicely.
Comment 7 Karsten Düsterloh 2009-07-10 16:38:04 PDT
Comment on attachment 385658 [details] [diff] [review]
Patch, version 1

Nice, just some nits: 

>diff --git a/mailnews/local/src/nsMovemailService.cpp b/mailnews/local/src/nsMovemailService.cpp
> PRBool ObtainSpoolLock(const char *spoolnameStr,
>-                       int seconds /* number of seconds to retry */)
>+                       int seconds /* number of seconds to retry */,
>+                       PRBool *usingLockFile)
> {

While you're touching these functions, you could make the parameters adhere to the Mozilla 'aSpoolName' naming scheme. (Same for YieldSpoolLock down below.)
And you should check 

>-  // How to lock:
>-  // step 1: create SPOOLNAME.mozlock
>-  //        1a: can remove it if it already exists (probably crash-droppings)
>-  // step 2: hard-link SPOOLNAME.mozlock to SPOOLNAME.lock for NFS atomicity
>-  //        2a: if SPOOLNAME.lock is >60sec old then nuke it from orbit
>-  //        2b: repeat step 2 until retry-count expired or hard-link succeeds
>-  // step 3: remove SPOOLNAME.mozlock
>-  // step 4: If step 2 hard-link failed, fail hard; we do not hold the lock
>-  // DONE.
>-  //
>-  // (step 2a not yet implemented)

Please don't let that comment die, just move it down after the new kernel locking.

>+  nsresult rv = NS_NewNativeLocalFile(nsDependentCString(spoolnameStr), PR_TRUE,
>+    getter_AddRefs(spoolFile));

Either use one line, ignoring its length, or put arguments 2 and 3 on their own line and align them vertically with nsDep...
(Plus one more instance further down.)

>+  if (!*usingLockFile)
>+  {

Please adhere to the prevalent bracing style of this file.
(Plus some more instances further down.)
Comment 8 Joshua Cranmer [:jcranmer] 2009-07-19 18:02:44 PDT
Pushed to comm-central as 3094:496bf05c0012.

Note You need to log in before you can comment on or make changes to this bug.