From 2ac68d82aaa2a3651662888cb3e9aff21c6f92a2 Mon Sep 17 00:00:00 2001 From: Nicolas Sebrecht Date: Mon, 24 Oct 2016 03:19:26 +0200 Subject: [PATCH] UIDMaps: prevent from leaving a truncated map file If the map file is not properly written (e.g. due to unexpected kill) offlineimap might wrongly consider some UIDs to have been deleted from the local side which could lead to data loss. Use a temporary map file rather than writing to the map file directly. Github-ref: https://github.com/OfflineIMAP/offlineimap/issues/380 Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/UIDMaps.py | 49 ++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/offlineimap/folder/UIDMaps.py b/offlineimap/folder/UIDMaps.py index 3259bad..cc4d84a 100644 --- a/offlineimap/folder/UIDMaps.py +++ b/offlineimap/folder/UIDMaps.py @@ -16,6 +16,8 @@ # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA import os.path +import shutil +from os import fsync, unlink from sys import exc_info from threading import Lock import six @@ -31,6 +33,7 @@ class MappedIMAPFolder(IMAPFolder): be an IMAPFolder. Instance variables (self.): + dofsync: boolean for fsync calls. r2l: dict mapping message uids: self.r2l[remoteuid]=localuid l2r: dict mapping message uids: self.r2l[localuid]=remoteuid #TODO: what is the difference, how are they used? @@ -39,18 +42,33 @@ class MappedIMAPFolder(IMAPFolder): def __init__(self, *args, **kwargs): IMAPFolder.__init__(self, *args, **kwargs) + self.dofsync = self.config.getdefaultboolean("general", "fsync", True) self.maplock = Lock() - (self.diskr2l, self.diskl2r) = self._loadmaps() + self.diskr2l, self.diskl2r = self._loadmaps() + self.r2l, self.l2r = None, None + # Representing the local IMAP Folder using local UIDs. + # XXX: This should be removed since we inherit from IMAPFolder. + # See commit 3ce514e92ba7 to know more. self._mb = IMAPFolder(*args, **kwargs) - """Representing the local IMAP Folder using local UIDs""" def _getmapfilename(self): return os.path.join(self.repository.getmapdir(), self.getfolderbasename()) def _loadmaps(self): - with self.maplock: - mapfilename = self._getmapfilename() + mapfilename = self._getmapfilename() + mapfilenametmp = "%s.tmp"% mapfilename + mapfilenamelock = "%s.lock"% mapfilename + with self.maplock and open(mapfilenamelock, 'w') as mapfilelock: + try: + fnctl.lockf(mapfilelock, fnctl.LOCK_EX) # Blocks until acquired. + except NameError: + pass # Windows... + if os.path.exists(mapfilenametmp): + self.ui.warn("a previous run might have leave the UIDMaps file" + " in incorrect state; some sync operations might be done" + " again and some emails might become duplicated.") + unlink(mapfilenametmp) if not os.path.exists(mapfilename): return ({}, {}) file = open(mapfilename, 'rt') @@ -77,8 +95,10 @@ class MappedIMAPFolder(IMAPFolder): def _savemaps(self): mapfilename = self._getmapfilename() + # Do not use the map file directly to prevent from leaving it truncated. + mapfilenametmp = "%s.tmp"% mapfilename mapfilenamelock = "%s.lock"% mapfilename - with open(mapfilenamelock, 'w') as mapfilelock: + with self.maplock and open(mapfilenamelock, 'w') as mapfilelock: # The "account" lock already prevents from multiple access by # different processes. However, we still need to protect for # multiple access from different threads. @@ -86,10 +106,13 @@ class MappedIMAPFolder(IMAPFolder): fnctl.lockf(mapfilelock, fnctl.LOCK_EX) # Blocks until acquired. except NameError: pass # Windows... - with open(mapfilename, 'wt') as mapfilefd: + with open(mapfilenametmp, 'wt') as mapfilefd: for (key, value) in self.diskl2r.items(): mapfilefd.write("%d:%d\n"% (key, value)) + if self.dofsync is True: + fsync(mapfilefd) # The lock is released when the file descriptor ends. + shutil.move(mapfilenametmp, mapfilename) def _uidlist(self, mapping, items): try: @@ -140,6 +163,7 @@ class MappedIMAPFolder(IMAPFolder): # Interface from BaseFolder def uidexists(self, ruid): """Checks if the (remote) UID exists in this Folder""" + # This implementation overrides the one in BaseFolder, as it is # much more efficient for the mapped case. return ruid in self.r2l @@ -147,7 +171,9 @@ class MappedIMAPFolder(IMAPFolder): # Interface from BaseFolder def getmessageuidlist(self): """Gets a list of (remote) UIDs. + You may have to call cachemessagelist() before calling this function!""" + # This implementation overrides the one in BaseFolder, as it is # much more efficient for the mapped case. return self.r2l.keys() @@ -155,16 +181,19 @@ class MappedIMAPFolder(IMAPFolder): # Interface from BaseFolder def getmessagecount(self): """Gets the number of messages in this folder. + You may have to call cachemessagelist() before calling this function!""" + # This implementation overrides the one in BaseFolder, as it is # much more efficient for the mapped case. return len(self.r2l) # Interface from BaseFolder def getmessagelist(self): - """Gets the current message list. This function's implementation - is quite expensive for the mapped UID case. You must call - cachemessagelist() before calling this function!""" + """Gets the current message list. + + This function's implementation is quite expensive for the mapped UID + case. You must call cachemessagelist() before calling this function!""" retval = {} localhash = self._mb.getmessagelist() @@ -208,6 +237,7 @@ class MappedIMAPFolder(IMAPFolder): check against dryrun settings, so you need to ensure that savemessage is never called in a dryrun mode. """ + self.ui.savemessage('imap', uid, flags, self) # Mapped UID instances require the source to already have a # positive UID, so simply return here. @@ -263,6 +293,7 @@ class MappedIMAPFolder(IMAPFolder): :param new_uid: The old remote UID will be changed to a new UID. The UIDMaps case handles this efficiently by simply changing the mappings file.""" + if ruid not in self.r2l: raise OfflineImapError("Cannot change unknown Maildir UID %s"% ruid, OfflineImapError.ERROR.MESSAGE)