<<< Date Index >>>     <<< Thread Index >>>

Re: crash in update_index()



On 2007-03-21 19:59:27 +0900, YONETANI Tomokazu wrote:

> I've been experiencing mutt segfaults when I open a Maildir
> folder and leave it a while.  I'm using some simple limit
> pattern, and the sorting is set to other than "threads".  New
> messages are delivered to this folder through procmail.  Older
> messages in that folder are periodically removed using cron job:
> 0 0 * * * find ~/foo/??? -type f -mtime +60 -delete

Mhhh...  That almost sounds like the crashes that I suddenly started
experiencing yesterday evening, but hadn't time to track down.  I'm
wondering a bit what recent change is exposing this problem.

> After playing with gdb, it turned out that update_index() tries to add
> messages satisfying the limit criteria to the end of v2r[], beyond
> Context->hdrmax.  The fix is as simple as this:
> 
> %%%%
> diff -r b0172175cc89 curs_main.c
> --- a/curs_main.c     Tue Mar 20 13:39:29 2007 -0700
> +++ b/curs_main.c     Wed Mar 21 19:40:47 2007 +0900
> @@ -288,6 +288,7 @@ static void update_index (MUTTMENU *menu
>    if (Context->pattern)
>    {
>  #define THIS_BODY Context->hdrs[j]->content
> +    Context->vcount = 0;

You're up to something there, but I think this fix is wrong.  (Then
again, it's been many years since I've last touched this part of the
code, so bear with me.)

I *think* the basic error condition is that the loop below starts at
j = 0, but vcount is > 0.  The other error condition would be to
reset vcount, but start with j > 0; in that case, new messages
wouldn't be shown in the index, I think.

Also, I'm wondering whether there is another condition for getting
into this loop where ctx->vcount might be incremented beyond
ctx->msgcount. Maybe time for an assert().

>      for (j = (check == M_REOPENED) ? 0 : oldcount; j < Context->msgcount; 
> j++)
>      {
>        if (mutt_pattern_exec (Context->limit_pattern,
> %%%%

> I also noticed that although update_index() has CONTEXT *ctx in its
> argument, it's not used and the global Context is used instead.
> Is this intended?

Woah.  Good catch.

I'm attaching an experimental patch.

-- 
Thomas Roessler   <roessler@xxxxxxxxxxxxxxxxxx>









diff -r b0172175cc89 curs_main.c
--- a/curs_main.c       Tue Mar 20 13:39:29 2007 -0700
+++ b/curs_main.c       Wed Mar 21 12:25:03 2007 +0100
@@ -49,6 +49,8 @@
 #include <sys/stat.h>
 #include <errno.h>
 
+#include <assert.h>
+
 static const char *No_mailbox_is_open = N_("No mailbox is open.");
 static const char *There_are_no_messages = N_("There are no messages.");
 static const char *Mailbox_is_read_only = N_("Mailbox is read-only.");
@@ -276,7 +278,7 @@ static void update_index (MUTTMENU *menu
   /* take note of the current message */
   if (oldcount)
   {
-    if (menu->current < Context->vcount)
+    if (menu->current < ctx->vcount)
       menu->oldcurrent = index_hint;
     else
       oldcount = 0; /* invalid message number! */
@@ -285,20 +287,24 @@ static void update_index (MUTTMENU *menu
   /* We are in a limited view. Check if the new message(s) satisfy
    * the limit criteria. If they do, set their virtual msgno so that
    * they will be visible in the limited view */
-  if (Context->pattern)
+  if (ctx->pattern)
   {
-#define THIS_BODY Context->hdrs[j]->content
-    for (j = (check == M_REOPENED) ? 0 : oldcount; j < Context->msgcount; j++)
+#define THIS_BODY ctx->hdrs[j]->content
+    for (j = (check == M_REOPENED) ? 0 : oldcount; j < ctx->msgcount; j++)
     {
-      if (mutt_pattern_exec (Context->limit_pattern,
+      if (!j)
+       ctx->vcount = 0;
+
+      if (mutt_pattern_exec (ctx->limit_pattern,
                             M_MATCH_FULL_ADDRESS, 
-                            Context, Context->hdrs[j]))
+                            ctx, ctx->hdrs[j]))
       {
-       Context->hdrs[j]->virtual = Context->vcount;
-       Context->v2r[Context->vcount] = j;
-       Context->hdrs[j]->limited = 1;
-       Context->vcount++;
-       Context->vsize += THIS_BODY->length + THIS_BODY->offset - 
THIS_BODY->hdr_offset;
+       assert (ctx->vcount < ctx->msgcount);
+       ctx->hdrs[j]->virtual = ctx->vcount;
+       ctx->v2r[ctx->vcount] = j;
+       ctx->hdrs[j]->limited = 1;
+       ctx->vcount++;
+       ctx->vsize += THIS_BODY->length + THIS_BODY->offset - 
THIS_BODY->hdr_offset;
       }
     }
 #undef THIS_BODY
@@ -308,13 +314,13 @@ static void update_index (MUTTMENU *menu
   if (oldcount && check != M_REOPENED
       && ((Sort & SORT_MASK) == SORT_THREADS))
   {
-    save_new = (HEADER **) safe_malloc (sizeof (HEADER *) * (Context->msgcount 
- oldcount));
-    for (j = oldcount; j < Context->msgcount; j++)
-      save_new[j-oldcount] = Context->hdrs[j];
+    save_new = (HEADER **) safe_malloc (sizeof (HEADER *) * (ctx->msgcount - 
oldcount));
+    for (j = oldcount; j < ctx->msgcount; j++)
+      save_new[j-oldcount] = ctx->hdrs[j];
   }
   
   /* if the mailbox was reopened, need to rethread from scratch */
-  mutt_sort_headers (Context, (check == M_REOPENED));
+  mutt_sort_headers (ctx, (check == M_REOPENED));
 
   /* uncollapse threads with new mail */
   if ((Sort & SORT_MASK) == SORT_THREADS)
@@ -323,31 +329,31 @@ static void update_index (MUTTMENU *menu
     {
       THREAD *h, *j;
       
-      Context->collapsed = 0;
+      ctx->collapsed = 0;
       
-      for (h = Context->tree; h; h = h->next)
+      for (h = ctx->tree; h; h = h->next)
       {
        for (j = h; !j->message; j = j->child)
          ;
-       mutt_uncollapse_thread (Context, j->message);
+       mutt_uncollapse_thread (ctx, j->message);
       }
-      mutt_set_virtual (Context);
+      mutt_set_virtual (ctx);
     }
     else if (oldcount)
     {
-      for (j = 0; j < Context->msgcount - oldcount; j++)
+      for (j = 0; j < ctx->msgcount - oldcount; j++)
       {
        int k;
        
-       for (k = 0; k < Context->msgcount; k++)
-       {
-         HEADER *h = Context->hdrs[k];
-         if (h == save_new[j] && (!Context->pattern || h->limited))
-           mutt_uncollapse_thread (Context, h);
+       for (k = 0; k < ctx->msgcount; k++)
+       {
+         HEADER *h = ctx->hdrs[k];
+         if (h == save_new[j] && (!ctx->pattern || h->limited))
+           mutt_uncollapse_thread (ctx, h);
        }
       }
       FREE (&save_new);
-      mutt_set_virtual (Context);
+      mutt_set_virtual (ctx);
     }
   }
   
@@ -355,9 +361,9 @@ static void update_index (MUTTMENU *menu
   if (oldcount)
   {
     /* restore the current message to the message it was pointing to */
-    for (j = 0; j < Context->vcount; j++)
+    for (j = 0; j < ctx->vcount; j++)
     {
-      if (Context->hdrs[Context->v2r[j]]->index == menu->oldcurrent)
+      if (ctx->hdrs[ctx->v2r[j]]->index == menu->oldcurrent)
       {
        menu->current = j;
        break;