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;