Wed 22 Jul 2009 03:46:45 PM UTC, original submission:
In ppp.c, err_t pppInit(void):
Recently I posted a mail and filed a bug about a possible leak
in pppInit() caused by
outpacket_buf[i] = (u_char *)mem_malloc(PPP_MRU+PPP_HDRLEN);
My further investigation shows that outpacket_buf is never deallocated, at least I could not find where. Correct me if I am wrong.
I beleive the right place to do this is just before the end of the ppp thread function:
static void
pppMain(void *arg)
{
...
p = pbuf_alloc(PBUF_RAW, PPP_MRU+PPP_HDRLEN, PBUF_RAM);
if (!p) {
LWIP_ASSERT("p != NULL", p);
pc->errCode = PPPERR_ALLOC;
goto out;
}
...
while (lcp_phase[pd] != PHASE_DEAD) {
...
sys_msleep(1); /* give other tasks a chance to run */
...
}
PPPDEBUG((LOG_INFO, "pppMain: unit %d: PHASE_DEADn", pd));
pppDrop(pc); /* bug fix #17726 */
pbuf_free(p);
out:
PPPDEBUG(...);
if(pc->linkStatusCB) {
pc->linkStatusCB(pc->linkStatusCtx, pc->errCode ? pc->errCode : PPPERR_PROTOCOL, NULL);
}
// ++++++++++++++++++++++ --->
LWIP_ASSERT("outpacket_buf is valid", (outpacket_buf[pd] != NULL));
//if (outpacket_buf[pd])// this should be here if I am not right about the ASSERT.
mem_free(outpacket_buf[pd]);
// +++++++++++++++++++++++ <---
pc->openFlag = 0;
}
I was thinking about adding mem_free somewhere in the callback function linkStatusCB,but it is not safe to do it because:
1. this may break someone's code, which assumes this buffer is valid and
2. this callback function is optional, the user has the freedom to define it or not. When such function is not present (grep for if(!linkStatusCB) and if(!pc->linkStatusCB)), usually (90%) the stack waits for (lcp_phase[pd] == PHASE_DEAD) and then calls pppClose(pd).
I thought about placing this mem_free in pppClose(), but then it may miss the PPPoE case and, worse, it will not work if the first pbuf_alloc in pppMain() fails.
I.e. this must be after the out: label.
Unfortunately, I can not test this since we do not use OS.
|