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

RESOLVED FIXED in Thunderbird 3.0b4

Status

MailNews Core
Movemail
--
major
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Sune Mølgaard, Assigned: jcranmer)

Tracking

Trunk
Thunderbird 3.0b4
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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)

Updated

10 years ago
Assignee: general → pkwarren
Component: General → MailNews: Movemail
Product: Mozilla Application Suite → Core
QA Contact: general → movemail
Version: unspecified → Trunk
Sune says "[Ubuntu] said to bug the mozilla dev team..."
Product: Core → MailNews Core
(Assignee)

Comment 2

9 years ago
Bug is confirmed on Linux Trunk.

This also seems to be a very, very cross-distro problem.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Updated

9 years ago
Duplicate of this bug: 239013
(Assignee)

Updated

9 years ago
Duplicate of this bug: 56671
(Assignee)

Comment 6

8 years ago
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.
Assignee: pkwarren → Pidgeot18
Status: NEW → ASSIGNED
Attachment #385658 - Flags: superreview?(bienvenu)
Attachment #385658 - Flags: review?(mnyromyr)

Updated

8 years ago
Attachment #385658 - Flags: superreview?(bienvenu) → superreview+

Comment 7

8 years ago
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.)
Attachment #385658 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 8

8 years ago
Pushed to comm-central as 3094:496bf05c0012.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b4

Updated

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