Re: [PATCH] Fix memory leaks
Hi,
* Rocco Rutte [06-07-07 09:25:24 +0000] wrote:
Brendan suggested to move the free call inside the '#ifdef HEADER_CACHE'-block as an else-part after the uid_validity check. I'll
report back if that helps and provide a patch for it if it does.
That doesn't work either. I don't know if my analysis is correct, but at
least the patch looks better now, IMHO.
As I don't know the IMAP code well/at all, first my assumptions gained
by reading the code.
The imap_read_headers() function reads a given amount of message
headers. The evil line 149 with the calloc() call is inside a block
trying to evaluate the hcache, i.e. verify that what is in the cache is
also present on the server. For each message the mailbox has, it tries
to fetch it from the hcache and assign the flags to the CONTEXT
structure, and, _important_, only does so while messages are in the
hcache. As I read it, it aborts for the message not in the hcache as
that one is believed to be the first totally new one ("totally" means
not even downloaded once; one can mark messages as new after reading
them for IMAP).
Now as my tests seem to proof, the leak only happens if a mailbox has
totally new mails. It could be nice if someone could verify that with
valgrind. Also, as the leaked memory is quite small per mailbox, I think
this proofs my assumption that the loop is supposed to abort for the
first totally new message.
And that is where I guess the leak is: even for such a message, memory
is allocated but obviously never used since there's nothing to fetch
from the hcache.
Attached are two patches.
The one with dprint() calls prints the code path inside
imap_read_headers(). The debug file shows that for the first totally new
mail, no "STEP" debug line will be printed but one line per known
message, _and_ the loop is stopped. But since we allocated memory, the
break statement is IMHO exactly where the leak is. It would be nice if
someone could try the patch-mem-verbose.diff patch with and without the
imap_free_header_data() call before printing the "STEP" debug line. With
the free call, my valgrind is happy but complains without it.
In case my analysis is right, the second patch contains just this free
call for inclusion in CVS. :)
And while I'm running valgrind, now I got another one:
==38018== 69 bytes in 5 blocks are definitely lost in loss record 9 of 57
==38018== at 0x3C03A19B: malloc (in
/usr/local/lib/valgrind/vgpreload_memcheck.so)
==38018== by 0x80A4072: safe_malloc (lib.c:146)
==38018== by 0x80A50B1: mutt_add_list_n (muttlib.c:246)
==38018== by 0x80C19A4: imap_open_mailbox (imap.c:616)
==38018== by 0x808104B: mx_open_mailbox (mx.c:718)
==38018== by 0x80787AE: main (main.c:974)
where muttlib.c:246 is:
head = tmp = safe_malloc (sizeof (LIST));
This is supposed to create the list if the given head is NULL. I don't
know in which direction these assignments are resolved, but since:
head = (tmp = ...);
and:
tmp = safe_malloc(); head = tmp;
still show a leak, I guess the code is fine (i.e. right to left
resolution) and the problem is somewhere else. But the handling of
idata->mboxcache looks okay to me, so I can't much here (yet).
bye, Rocco
--
:wq!
diff --git a/imap/message.c b/imap/message.c
index f9edfb4..b1d8fa1 100644
--- a/imap/message.c
+++ b/imap/message.c
@@ -153,7 +153,13 @@ #if USE_HCACHE
rc = imap_cmd_step (idata);
if (rc != IMAP_CMD_CONTINUE)
- break;
+ {
+ /* for the first totally unknown message, the loop will stop and
+ * we have to release the previously allocated memory for this
+ * last message to be processed to not leak memory */
+ imap_free_header_data ((void**) &h.data);
+ break;
+ }
if ((mfhrc = msg_fetch_header (ctx, &h, idata->buf, NULL)) == -1)
continue;
diff --git a/muttlib.c b/muttlib.c
diff --git a/imap/message.c b/imap/message.c
index f9edfb4..77ddac9 100644
--- a/imap/message.c
+++ b/imap/message.c
@@ -147,24 +147,35 @@ #if USE_HCACHE
memset (&h, 0, sizeof (h));
h.data = safe_calloc (1, sizeof (IMAP_HEADER_DATA));
+ dprint(1,(debugfile,"CALLOC: %d\n",msgno));
do
{
mfhrc = 0;
+ dprint(1,(debugfile,"LOOP: %d\n",msgno));
+
rc = imap_cmd_step (idata);
- if (rc != IMAP_CMD_CONTINUE)
+ if (rc != IMAP_CMD_CONTINUE) {
+ dprint(1,(debugfile,"BREAK: %d, rc=%d\n",msgno,rc));
+ imap_free_header_data ((void**) &h.data);
break;
+ }
+
+ dprint(1,(debugfile,"STEP: %d\n",msgno));
if ((mfhrc = msg_fetch_header (ctx, &h, idata->buf, NULL)) == -1)
continue;
else if (mfhrc < 0)
break;
+ dprint(1,(debugfile,"FETCH: %d\n",msgno));
+
sprintf(uid_buf, "/%u", h.data->uid); /* XXX --tg 21:41 04-07-11 */
uid_validity = (unsigned int*)mutt_hcache_fetch (hc, uid_buf,
&imap_hcache_keylen);
if (uid_validity != NULL && *uid_validity == idata->uid_validity)
{
+ dprint(1,(debugfile,"MANGLE: %d\n",msgno));
ctx->hdrs[msgno] = mutt_hcache_restore((unsigned char *)
uid_validity, 0);
ctx->hdrs[msgno]->index = h.sid - 1;
if (h.sid != ctx->msgcount + 1)
@@ -182,15 +193,21 @@ #if USE_HCACHE
ctx->hdrs[msgno]->data = (void *) (h.data);
ctx->msgcount++;
- }
-
+ } else {
+ dprint(1,(debugfile,"FAIL: %d\n",msgno));
+ }
+
FREE(&uid_validity);
}
while (rc != IMAP_CMD_OK && mfhrc == -1);
+
+ dprint(1,(debugfile,"ENDLOOP: %d, rc=%d, mfhrc=%d\n",msgno,rc,mfhrc));
+
if (rc == IMAP_CMD_OK)
break;
if ((mfhrc < -1) || ((rc != IMAP_CMD_CONTINUE) && (rc != IMAP_CMD_OK)))
{
+ dprint(1,(debugfile,"RETURN -1: %d\n",msgno));
imap_free_header_data ((void**) &h.data);
fclose (fp);
mutt_hcache_close (hc);
diff --git a/muttlib.c b/muttlib.c