Re: [bug-mailutils] mailbox corrupted

[Sergey Poznyakoff]

Fredrick O Jackson <address@hidden> wrote:

> 
> Bug is reliably reproduced.
> 
> I am using Kmail to access this account. have yet to determine if using 
> outlook makes any difference. I have determined that I accessed the mailbox 
> while another client was connected to the same mailbox. is this an issue?
> 

Yes, this can be it. Imap4d was not able to cope with such third-party
modifications to the mailbox that decrease its size (by removing or
editing some messages). I have installed the attached patch. It
should apply cleanly to version 0.6. After applying the patch, imap4d
will issue untagged response whenever the mailbox is shrunk by another
program. If the client is able to handle it, it will update its data.
If not, it will close the mailbox (mutt appears to prefer this way).
In any case, mailbox will not be corrupted.

There are two issues, though:

1. If a client updates any messages (by updating their flags or
contents) and these changes are not flushed before the mailbox
is shrunk by another program, such changes will be lost.
This is a reasonable price for preserving mailbox contents.

2. If the third-party program edits the mailbox in a way that increases
its size (say, by appending an extra header to any message), imap4d
is not able to tell such changes from appending a new message and the
mailbox corruption can still occur. This case is being worked upon.

Anyways, modifying the mailbox, except appending new messages
to it, while it is being worked upon by another program is not a good
idea...

Let me know if you experience any problems with the patch.

Regards,
Sergey

2005-02-16  Sergey Poznyakoff <address@hidden>

        Fix a long-standing issue in mailbox and imap4d:
        be able to handle a case when a third-party program deletes
        some messages in the mailbox being open by imap4d (imap4d
        cannot and should not lock it, hence the possibility).
        
        * mailbox/amd.c (amd_pool_flush): New function.
        (amd_close): Call amd_pool_flush()
        and initialize amd data so that subsequent amd_open() works
        properly.
        (amd_destroy): Call amd_pool_flush.
        * mailbox/mbox/mbox.c (mbox_close): Initialize all private data
        so that subsequent mailbox_open() works properly.
        (mbox_is_updated): Updated comment

        * imap4d/bye.c (imap4d_bye0): Handle ERR_MAILBOX_CORRUPTED
        * imap4d/imap4d.h (ERR_MAILBOX_CORRUPTED): New error code.
        (imap4d_set_observer): New function
        * imap4d/select.c (imap4d_select0): Call imap4d_set_observer()
        * imap4d/sync.c (imap4d_set_observer): New function.
        (imap4d_sync): Check if the mailbox was shrunk (in libmailbox
        terms 'corrupted'). If so, reopen it and issue untagged alert.

Index: imap4d/bye.c
===================================================================
RCS file: /cvsroot/mailutils/mailutils/imap4d/bye.c,v
retrieving revision 1.14
diff -p -u -r1.14 bye.c
--- imap4d/bye.c        29 Aug 2003 19:08:08 -0000      1.14
+++ imap4d/bye.c        16 Feb 2005 12:39:37 -0000
@@ -58,6 +58,10 @@ imap4d_bye0 (int reason, struct imap4d_c
       syslog (LOG_INFO, _("No socket to send to"));
       break;
 
+    case ERR_MAILBOX_CORRUPTED:
+      syslog (LOG_ERR, _("Mailbox modified by third party"));
+      break;
+      
     case OK:
       util_out (RESP_BYE, "Session terminating.");
       if (state == STATE_NONAUTH)
Index: imap4d/imap4d.h
===================================================================
RCS file: /cvsroot/mailutils/mailutils/imap4d/imap4d.h,v
retrieving revision 1.59
diff -p -u -r1.59 imap4d.h
--- imap4d/imap4d.h     15 Nov 2004 18:44:34 -0000      1.59
+++ imap4d/imap4d.h     16 Feb 2005 12:39:38 -0000
@@ -128,13 +128,14 @@ struct imap4d_command
 #define RESP_NONE      4
 
 /* Error values.  */
-#define OK 0
-#define ERR_NO_MEM 1
-#define ERR_NO_OFILE 2
-#define ERR_TIMEOUT 3
-#define ERR_SIGNAL 4
-#define ERR_TLS 5
-
+#define OK                    0
+#define ERR_NO_MEM            1
+#define ERR_NO_OFILE          2
+#define ERR_TIMEOUT           3
+#define ERR_SIGNAL            4
+#define ERR_TLS               5
+#define ERR_MAILBOX_CORRUPTED 6
+  
 /* Namespace numbers */
 #define NS_PRIVATE 0
 #define NS_OTHER   1
@@ -216,7 +217,8 @@ extern void fetch_flags0 (const char *pr
 extern int imap4d_sync __P ((void));
 extern int imap4d_sync_flags __P ((size_t));
 extern size_t uid_to_msgno __P ((size_t));
-
+extern void imap4d_set_observer (mailbox_t mbox);
+  
 /* Signal handling.  */
 extern RETSIGTYPE imap4d_sigchld __P ((int));
 extern RETSIGTYPE imap4d_signal __P ((int));
Index: imap4d/select.c
===================================================================
RCS file: /cvsroot/mailutils/mailutils/imap4d/select.c,v
retrieving revision 1.28
diff -p -u -r1.28 select.c
--- imap4d/select.c     9 Feb 2003 12:33:01 -0000       1.28
+++ imap4d/select.c     16 Feb 2005 12:39:38 -0000
@@ -1,5 +1,5 @@
 /* GNU Mailutils -- a suite of utilities for electronic mail
-   Copyright (C) 1999, 2001 Free Software Foundation, Inc.
+   Copyright (C) 1999, 2001, 2003, 2005 Free Software Foundation, Inc.
 
    GNU Mailutils is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -64,6 +64,9 @@ imap4d_select0 (struct imap4d_command *c
     {
       select_flags = flags;
       state = STATE_SEL;
+
+      imap4d_set_observer (mbox);
+      
       if ((status = imap4d_select_status ()) == 0)
        {
          free (mailbox_name);
Index: imap4d/sync.c
===================================================================
RCS file: /cvsroot/mailutils/mailutils/imap4d/sync.c,v
retrieving revision 1.9
diff -p -u -r1.9 sync.c
--- imap4d/sync.c       9 Feb 2003 12:33:01 -0000       1.9
+++ imap4d/sync.c       16 Feb 2005 12:39:38 -0000
@@ -16,6 +16,7 @@
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA  */
 
 #include "imap4d.h"
+#include <mailutils/observer.h>
 
 /*
 
@@ -271,6 +272,38 @@ imap4d_sync_flags (size_t msgno)
   return 0;
 }
 
+static int mailbox_corrupt;
+
+static int
+action (observer_t observer, size_t type)
+{
+  switch (type)
+    {
+    case MU_EVT_MAILBOX_CORRUPT:
+      mailbox_corrupt = 1;
+      break;
+
+    case MU_EVT_MAILBOX_DESTROY:
+      mailbox_corrupt = 0;
+      break;
+    }
+  return 0;
+}
+
+void
+imap4d_set_observer (mailbox_t mbox)
+{
+  observer_t observer;
+  observable_t observable;
+      
+  observer_create (&observer, mbox);
+  observer_set_action (observer, action, mbox);
+  mailbox_get_observable (mbox, &observable);
+  observable_attach (observable, MU_EVT_MAILBOX_CORRUPT|MU_EVT_MAILBOX_DESTROY,
+                    observer);
+  mailbox_corrupt = 0;
+}
+
 int
 imap4d_sync (void)
 {
@@ -280,7 +313,25 @@ imap4d_sync (void)
   if (mbox == NULL)
     free_uids ();
   else if (!uid_table_loaded || !mailbox_is_updated (mbox))
-    notify ();
+    {
+      if (mailbox_corrupt)
+       {
+         /* Some messages have been deleted from the mailbox by some other
+            party */
+         int status = mailbox_close (mbox);
+         if (status)
+           imap4d_bye (ERR_MAILBOX_CORRUPTED);
+         status = mailbox_open (mbox, MU_STREAM_RDWR);
+         if (status)
+           imap4d_bye (ERR_MAILBOX_CORRUPTED);
+         imap4d_set_observer (mbox);
+         free_uids ();
+         mailbox_corrupt = 0;
+         util_out (RESP_NONE,
+                   "OK [ALERT] Mailbox modified by another program");
+       }
+      notify ();
+    }
   else
     {
       size_t count = 0;
Index: mailbox/amd.c
===================================================================
RCS file: /cvsroot/mailutils/mailutils/mailbox/amd.c,v
retrieving revision 1.7
diff -p -u -r1.7 amd.c
--- mailbox/amd.c       2 Jun 2004 11:03:35 -0000       1.7
+++ mailbox/amd.c       16 Feb 2005 12:39:38 -0000
@@ -105,6 +105,7 @@ static void _amd_message_delete __P((str
                                    struct _amd_message *msg));
 static int amd_pool_open __P((struct _amd_message *mhm));
 static int amd_pool_open_count __P((struct _amd_data *amd));
+static void amd_pool_flush (struct _amd_data *amd);
 static struct _amd_message **amd_pool_lookup __P((struct _amd_message *mhm));
 
 static int amd_envelope_date __P((envelope_t envelope, char *buf, size_t len,
@@ -303,6 +304,7 @@ amd_destroy (mailbox_t mailbox)
   if (!amd)
     return;
 
+  amd_pool_flush (amd);
   monitor_wrlock (mailbox->monitor);
   for (i = 0; i < amd->msg_count; i++)
     {
@@ -340,8 +342,30 @@ amd_open (mailbox_t mailbox, int flags)
 static int
 amd_close (mailbox_t mailbox)
 {
+  struct _amd_data *amd;
+  int i;
+    
   if (!mailbox)
     return MU_ERR_MBX_NULL;
+
+  amd = mailbox->data;
+  
+  /* Destroy all cached data */
+  amd_pool_flush (amd);
+  monitor_wrlock (mailbox->monitor);
+  for (i = 0; i < amd->msg_count; i++)
+    {
+      message_destroy (&amd->msg_array[i]->message, amd->msg_array[i]);
+      free (amd->msg_array[i]);
+    }
+  free (amd->msg_array);
+  amd->msg_array = NULL;
+
+  amd->msg_count = 0; /* number of messages in the list */
+  amd->msg_max = 0;   /* maximum message buffer capacity */
+
+  amd->uidvalidity = 0;
+
   return 0;
 }
 
@@ -1124,6 +1148,21 @@ amd_pool_open (struct _amd_message *mhm)
   return 0;
 }
 
+static void
+amd_pool_flush (struct _amd_data *amd)
+{
+  int i;
+  
+  for (i = amd->pool_first; i != amd->pool_last; )
+    {
+      if (amd->msg_pool[i])
+       amd_message_stream_close (amd->msg_pool[i]);
+      if (++i == MAX_OPEN_STREAMS)
+       i = 0;
+    }
+  amd->pool_first = amd->pool_last = 0;
+}
+
 /* Attach a stream to a given message structure. The latter is supposed
    to be already added to the open message pool. */
 int
Index: mailbox/mbox/mbox.c
===================================================================
RCS file: /cvsroot/mailutils/mailutils/mailbox/mbox/mbox.c,v
retrieving revision 1.79
diff -p -u -r1.79 mbox.c
--- mailbox/mbox/mbox.c 3 Jun 2004 21:44:10 -0000       1.79
+++ mailbox/mbox/mbox.c 16 Feb 2005 12:39:38 -0000
@@ -277,7 +277,7 @@ static int
 mbox_close (mailbox_t mailbox)
 {
   mbox_data_t mud = mailbox->data;
-  /* size_t i; */
+  size_t i; 
 
   if (mud == NULL)
     return EINVAL;
@@ -287,10 +287,14 @@ mbox_close (mailbox_t mailbox)
   /* Make sure that we do not hold any file locking.  */
   locker_unlock (mailbox->locker);
 
-#if 0
-  /* RFC: I'm not sure on the right approach especially if the client is
+  /* Alain: I'm not sure on the right approach especially if the client is
      working in disconnected mode, where it can mailbox_close/mailbox_open
-     for each request, maybe we should keep them for a while.  */
+     for each request, maybe we should keep them for a while.
+
+     Sergey: No, it actually breaks reopening the mailbox. We should make
+     sure that the sequence mailbox_open();mailbox_close() will catch all
+     the changes that might have been done to the mailbox */
+  
   monitor_wrlock (mailbox->monitor);
   /* Before closing we need to remove all the messages
      - to reclaim the memory
@@ -317,7 +321,7 @@ mbox_close (mailbox_t mailbox)
   mud->uidvalidity = 0;
   mud->uidnext = 0;
   monitor_unlock (mailbox->monitor);
-#endif
+
   return stream_close (mailbox->stream);
 }
 
@@ -346,11 +350,17 @@ mbox_scan (mailbox_t mailbox, size_t msg
   return 0;
 }
 
-/* FIXME:  How to handle a shrink ? meaning, the &address@hidden@^& user start 
two
+/* Alain: How to handle a shrink ? meaning, the &address@hidden@^& user start 
two
    browsers and deleted emails in one session.  My views is that we should
    scream bloody murder and hunt them with a machette. But for now just play
    dumb, but maybe the best approach is to pack our things and leave
-   .i.e exit()/abort().  */
+   .i.e exit()/abort(). 
+
+   Sergey: Nope, we shouldn't abort. Handling it with MU_EVT_MAILBOX_CORRUPT
+   is sensible enough. The caller must decide what's the best approach
+   in this case. The simplest one is reopening the mailbox. Imap4d currently
+   does that. */
+
 static int
 mbox_is_updated (mailbox_t mailbox)
 {