Re: mutt 1.5.8i: SIGSEGV in imap_sync_mailbox
Hello Brendan,
> > diff --git a/hash.c b/hash.c
> > +++ b/hash.c
> > @@ -114,8 +114,12 @@ void *hash_find_hash (const HASH * table
> > void hash_delete_hash (HASH * table, int hash, const char *key, const void
> > *data,
> > void (*destroy) (void *))
> > {
> > - struct hash_elem *ptr = table->table[hash];
> > - struct hash_elem **last = &table->table[hash];
> > + struct hash_elem *ptr;
> > + struct hash_elem **last;
> > +
> > +again:
> > + ptr = table->table[hash];
> > + last = &table->table[hash];
> > for (; ptr; last = &ptr->next, ptr = ptr->next)
> > {
> > @@ -128,7 +132,7 @@ void hash_delete_hash (HASH * table, int
> > *last = ptr->next;
> > if (destroy) destroy (ptr->data);
> > FREE (&ptr);
> > - return;
> > + goto again;
> > }
> > }
> > }
> this changes the semantics of hash_delete. Is it safe to do this? I
> haven't audited the code myself...
the problem was that the 'data' was referenced in more than one entry,
but only the first one was deleted. So deleting the first key which was
pointing to data didn't do the job. But doing it the above way should
be safe. I had another version which should obtain the same which
segfaulted this one works for me.
> > diff --git a/imap/message.c b/imap/message.c
> > +++ b/imap/message.c
> > @@ -314,7 +314,10 @@ int imap_read_headers (IMAP_DATA* idata,
> > fclose(fp);
> > if (ctx->msgcount > oldmsgcount)
> > + {
> > + mx_alloc_memory(ctx);
> > mx_update_context (ctx, ctx->msgcount - oldmsgcount);
> > + }
> > return msgend;
> > }
> Can you explain why this is necessary? It looks like there should
> already be enough memory by this point.
I have a valgrind log which tells me that there wasn't enough space
available when calling mx_update_context.
> > @@ -469,6 +472,8 @@ int imap_fetch_message (MESSAGE *msg, CO
> > hash_delete (ctx->id_hash, h->env->message_id, h, NULL);
> > if (ctx->subj_hash && h->env->real_subj)
> > hash_delete (ctx->subj_hash, h->env->real_subj, h, NULL);
> > + if (ctx->thread_hash && h->env->message_id)
> > + hash_delete (ctx->thread_hash, h->env->message_id, NULL, NULL);
> > mutt_free_envelope (&h->env);
> > h->env = mutt_read_rfc822_header (msg->fp, h, 0, 0);
> > if (ctx->id_hash && h->env->message_id)
> This is probably the important part? Do you think it should be in
> mx_update_tables as well?
I didn't know about mx_update_tables before you told me about it.
However putting it in there doesn't hurt, so I think *yes* it should be
in there, too. But this one with the above (delete all references to
data) does the job, I think.
Thomas