Fri 10 Aug 2007 05:29:54 AM UTC, original submission:
I'm looking at the "head" revision of ppp.c (probably Revision 1.13) in the CVS repository.
I'm just reviewing the code (comparing the current head revision with the 1.2.0 sources I'm working from) and not finding problems from running the code (yet).
I think I've found some pbuf leaks.
pppInput is called from pppInProc() as follows:
/* Dispatch the packet thereby consuming it. */
if(tcpip_callback(pppInput, pc->inHead) != ERR_OK) {
That clearly states that pppInput() has to consume or dispose of the buffer.
The very first test in pppInput() is:
if(pbuf_header(nb, -(int)sizeof(struct pppInputHeader))) {
/* Can we cope with this failing? Just assert for now */
LWIP_ASSERT("pbuf_header failed\n", 0);
return;
}
That's a new addition in Revision 1.11. I'm pretty sure the above should NOT "return", but should "goto drop" like most of the others do, as the "drop:" and "out:" flags lead to code that disposes of the buffer.
The second test (of lcp_phase[] and other things) performs "goto drop".
The third one:
if (vj_uncompress_tcp(&nb, &pppControl[pd].vjComp) >= 0) {
if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
}
return;
}
also looks like it could leak a pbuf and should probably be:
if (vj_uncompress_tcp(&nb, &pppControl[pd].vjComp) >= 0) {
if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
return;
} else {
goto drop;
}
}
The fourth one is the same as the third one, but calls vj_uncompress_uncomp() instead; same fix applies.
The fifth one looks to have the same bug:
case PPP_IP: /* Internet Protocol */
PPPDEBUG((LOG_INFO, "pppInput[%d]: ip in pbuf
len=%d\n", pd, nb->len));
if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
}
return;
The last four lines should be:
if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
return;
} else {
goto drop;
}
That's the end of the pbuf leaks that I can see on a code inspection.
While we're here:
/*
* Upcall the proper protocol input routine.
*/
for (i = 0; (protp = ppp_protocols[i]) != NULL; ++i) {
if (protp->protocol == protocol && protp->enabled_flag) {
...
would be a little more robust if it included a check on the function pointer as follows before it uses the pointer:
if (protp->protocol == protocol && protp->enabled_flag &&
protp->input != NULL) {
...
|