384774 - Mailnews:movemail. Fails in the increasingly common situation of no write privs in mail spool dir

Status: RESOLVED FIXED

(MailNews Core :: Movemail, defect)

Description

[Sune Mølgaard]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.4) Gecko/20070509 SeaMonkey/1.1.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.1.4) 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 https://bugzilla.mozilla.org/show_bug.cgi?id=56671 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 (https://bugs.launchpad.net/ubuntu/+source/mozilla-thunderbird/+bug/96566)

Comment 1

[Wayne Mery (:wsmwk)]

Sune says "[Ubuntu] said to bug the mozilla dev team..."

Comment 2

[Joshua Cranmer [:jcranmer]]

Bug is confirmed on Linux Trunk.

This also seems to be a very, very cross-distro problem.

Comment 3

[Joshua Cranmer [:jcranmer]]

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 6

[Joshua Cranmer [:jcranmer]]

Attachment: 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]

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 
  NS_ENSURE_ARG_POINTER(aUsingLockFile);
here.

>-  // 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]]

Pushed to comm-central as 3094:496bf05c0012.