Bug 3212 - double free in message_cleanup
Summary: double free in message_cleanup
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamc/spamd (show other bugs)
Version: 2.63
Hardware: Other other
: P5 trivial
Target Milestone: 3.0.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-03-24 13:59 UTC by dick wesseling
Modified: 2004-04-04 11:23 UTC (History)
0 users


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description dick wesseling 2004-03-24 13:59:17 UTC 
I intermittently get a bad heap in spamc when running the test suite.
It seems to be caused by this:

message_read_bsmtp() sets m->msg to point *somewhere* in m->raw:

            i+=6;
            if(m->raw[i-1]=='\r') i++;
            m->pre_len=i;
            m->msg=m->raw+i;
                         ^^

and message_cleanup() attempts to avoid double freeing by
testing for (m->out != m->raw) without considering the "i" added
by message_read_bsmtp():


void message_cleanup(struct message *m) {
   if (m->out != NULL && m->out != m->raw) free(m->out);
   if (m->raw != NULL) free(m->raw);
   if (m->priv != NULL) free(m->priv);
   clear_message(m);



This is low priority bug. The root of the problem was the the account "nobody"
had disappeared. This caused the daemon to shut down prematurely which probably
caused spamc to exercise a rare path through the code. Once I had restored the
"nobody" account the test worked.
Comment 1 Theo Van Dinter 2004-03-30 19:45:53 UTC 
stupid C and it's memory handling!  ;)  nice catch though.

original:

    if (m->out != NULL && m->out != m->raw)
        free(m->out);
    if (m->raw != NULL)
        free(m->raw);

could be changed to:

    if (m->out != NULL && m->pre != NULL && m->out != m->pre+m->pre_len)
        free(m->out);
    if (m->raw != NULL)
        free(m->raw);

from my investigation of the code, m->out will end up getting a malloc() call which will keep it out of 
the way of m->pre and m->pre_len.  but as far as I can tell, m->pre and m->pre_len only get defined 
and set in message_read_bsmtp, so in theory, they should be safe.

thoughts?

BTW: as a clarification for the above...  by default, raw=null, out=null, pre=null, pre_len=0.  in 
message_read_raw, out=raw.  in message_read_bsmtp, out=raw+i, pre=raw, pre_len=i.  so in theory, if 
pre==null, then out==raw, so don't free.  if pre!=null, we can see if out==pre+pre_len, which would 
be the same as out==raw+i, which is how it's set in message_read_bsmtp.

so I think that's all right, but my C is very rusty.
Comment 2 Theo Van Dinter 2004-04-04 19:23:41 UTC 
ok, I've applied the patch. :)  r9864