Bug #202

Mbox locking not NFS-safe

Added by Ricardo Mones 5 months ago. Updated 4 months ago.

Status:ResolvedStart date:05/20/2014
Priority:HighDue date:
Assignee:Hiroyuki Yamamoto% Done:

0%

Category:SylpheedSpent time:-
Target version:3.5

Description

I've been investigating bug #198 a bit and seems it seems worse: Sylpheed locking method is not NFS-safe.

If you look in libsylph/mbox.c:lock_mbox function, you can see there's a variety of locking methods implemented, but looking at inc.c:get_spool function only LOCK_FLOCK is attempted.

As detailed for example in https://www.spinnaker.de/linux/nfs-locking.html, the correct way would be to try LOCK_LOCKF then dotlocking if the former fails.

Note that current locking goes also against Debian policy 11.6 https://www.debian.org/doc/debian-policy/ch-customized-programs.html#s-mail-transport-agents wich requires MUAs to lock the mailbox in an NFS-safe way.

Thanks in advance,

0001-use-lockfile_-for-NFS-safe-lock-202.patch Magnifier (4.4 KB) Kentaro HAYASHI, 06/03/2014 02:16 AM

use-lockf-or-liblockfile-rather-than-flock.patch Magnifier (3.7 KB) Hiroyuki Yamamoto, 06/04/2014 11:20 AM


Related issues

Related to Bug #198: document mbox locking method New 04/30/2014

History

#1 Updated by Ricardo Mones 5 months ago

Hi,

Notice that if this remain unfixed Sylpheed package will be automatically removed from Debian testing in around 11 days.

thanks,

#2 Updated by Kentaro HAYASHI 5 months ago

I've attached a patch which use liblockfile.

Mr Yamamoto, please review it.

#3 Updated by Kentaro HAYASHI 5 months ago

  • Status changed from Confirmed to In Progress

#4 Updated by Kentaro HAYASHI 5 months ago

  • Related to Bug #198: document mbox locking method added

#5 Updated by Hiroyuki Yamamoto 5 months ago

I have checked your patch.

1. Filename passed to lockfile_create() should be base + ".lock" ?
2. liblockfile just seems to create a lock file,
so lockf() (fcntl) also should be combined?

I'm not sure about the above, because I have never used liblockfile.

#6 Updated by Ricardo Mones 5 months ago

Hi Hiroyuki,

From the lockfile_create manual page (http://linux.die.net/man/3/lockfile_create) you can confirm the answer to your questions is yes: "One, it just creates a lockfile- it doesn't know which file you are actually trying to lock!"

regards,

#7 Updated by Hiroyuki Yamamoto 5 months ago

I have created a patch loosely based on HAYASHI's one:

1. use lockf() rather than flock()
2. use liblockfile if 1. fails

Liblockfile replaces the current file locking implementation,
because it doesn't work because of permission.
lockfile_check() is omitted since g_timeout_add() doesn't work in this context.

Ricardo, can you test this patch and apply if it's okay?

#8 Updated by Ricardo Mones 5 months ago

Hi Hiroyuki,

Many thanks for the patch! As far as I can see it is correct: implements mbox locking in the recommended order, also giving priority to lockf (fnctl) over flock on systems which have both (like Debian systems).

Are you planning to do a new release soon?

Just asking to see if it's worth to port the patch to current Debian package or is simply better to wait a few days for the new release.

#9 Updated by Hiroyuki Yamamoto 5 months ago

Are you planning to do a new release soon?

I will not be able to release a new version including this modification soon.

#10 Updated by Kentaro HAYASHI 5 months ago

I've completely misunderstood behaviour of liblockfile, anyway, thanks Mr. Yamamoto!

#11 Updated by Hiroyuki Yamamoto 4 months ago

  • Status changed from In Progress to Resolved

Applied at 3.5.0beta1.

(though I don't recommend to include beta versions in official distribution repository,
since they tend to contain incomplete implementation)

#12 Updated by Ricardo Mones 4 months ago

Hiroyuki Yamamoto wrote:

Applied at 3.5.0beta1.

Thanks! :)

(though I don't recommend to include beta versions in official distribution
repository, since they tend to contain incomplete implementation)

In Debian betas are uploaded to sid (unstable) in order to gain wider exposure and detect possible problems. They also migrate to the testing distribution according to the usual rules (time enough, no RC bugs). At the time of freeze it may happen a beta is the one in testing, therefore it may be released in the stable distribution.

By your comment I'm not sure if you would prefer another kind of management or not.

It's possible to upload betas just to experimental, and only final versions to unstable to ensure only final versions are distributed in stable. But this has also important drawbacks.

Just let me know. You can just mail me privately if you prefer or even on the mailing list if you want a public discussion.

Also available in: Atom PDF