buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19729, pbuf_free problem

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #19729: pbuf_free problem

Submitter:  Cui hengbin <phant>
Submitted:  Sat 28 Apr 2007 08:28:21 AM UTC
   
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Thu 17 May 2007 09:06:57 AM UTC, comment #9: 

You're right. Done that. Reading the code I ask myself why ip_input has a return type of err_t instead of void since it always returns ERR_OK and frees the pbuf itself.

OK, but now I'll really close it as fixed.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 16 May 2007 08:59:04 PM UTC, comment #8: 

So, if current ethernetif.c is good, you should remove pbuf_free(p)  from tcpip_input & tcpip_ethinput in tcpip.c, when "msg = memp_malloc(MEMP_TCPIP_MSG);" failed, like you have propose in comment #2, no ?

(be careful to tabs)

Frédéric Bernon <fbernon>
Group Member
Wed 16 May 2007 07:55:04 PM UTC, comment #7: 

ethernetif.c is already up to date, checked in the others.

What remains is ppp.c: return value is not checked but pbuf is freed all the time (regardless of the return value of ip_input, which returns ERR_OK all the time, anyway. Personally, I don't think that's right, but as I don't know/understand the ppp code anyway, I think it is not really related to this bug).

Closing this as fixed.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 16 May 2007 07:01:14 PM UTC, comment #6: 

Ok, no problem for me !!! Can you just also update ethernetif.c ?

Frédéric Bernon <fbernon>
Group Member
Wed 16 May 2007 06:58:54 PM UTC, comment #5: 

Frédéric, can I check in loopif.c, ppp.c & slipif.c that check netif->input() return value and delete p if != NULL?

It's assigned to you, but I already have it changed locally, so...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 30 Apr 2007 11:52:05 AM UTC, comment #4: 

So I think we should update loopif to free the pbuf if netif->input() fails!

PPP also calls netif->input() without checking the return value. That would have to be changed. However, as long as ip_input() is used, that doesn't matter since ip_input() always returns ERR_OK and frees the pbuf itself on error.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 29 Apr 2007 09:22:41 PM UTC, comment #3: 

I'm agree With Simon is we only study ethernetif use, but I'm not sure if loopif and ppp interface handle that correctly :

In loopif, no check is done, and in ppp, I don't know...



Frédéric Bernon <fbernon>
Group Member
Sun 29 Apr 2007 10:30:46 AM UTC, comment #2: 

I think it would be better to change tcpip.c (tcpip_input() and tcpip_ethinput()) not to free the pbuf but simply returning != ERR_OK and letting the ethernet driver free the pbuf. I think that's more consistent behaviour.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 29 Apr 2007 03:29:30 AM UTC, comment #1: 


so sorry ,in ethernetif.c should change as followed


if (netif->input(p, netif)==ERR_VAL)      
         { LWIP_DEBUGF(NETIF_DEBUG, ("ethernetif_init: IP input error\n"));
       pbuf_free(p);                                                     
       p = NULL;
     }

Cui hengbin <phant>
Sat 28 Apr 2007 08:28:21 AM UTC, original submission:  

Hi ALL:
       When I define ETHARP_TCPIP_ETHINPUT , using sniffer generating broadcast frame to network,there always assert at line 606 in pbuf.c .
        But I find it a bug,
            in tcpip.c  when no memory can malloc ,free it ,but in ethernetif.c ethernetif_input free it again.
                   
#if ETHARP_TCPIP_ETHINPUT
err_t
tcpip_ethinput(struct pbuf *p, struct netif *inp)
{
  struct tcpip_msg *msg;
 
  if (mbox != SYS_MBOX_NULL) {
    msg = memp_malloc(MEMP_TCPIP_MSG);
    if (msg == NULL) {
      pbuf_free(p);                                                                  free here
      return ERR_MEM; 
    }
   
    msg->type = TCPIP_MSG_ETHINPUT;
    msg->msg.inp.p = p;
    msg->msg.inp.netif = inp;
    sys_mbox_post(mbox, msg);
    return ERR_OK;
  }
  return ERR_VAL;
}
#endif /* ETHARP_TCPIP_ETHINPUT */
 
 
static void
ethernetif_input(struct netif *netif)
{
  struct ethernetif *ethernetif;
  struct eth_hdr *ethhdr;
  struct pbuf *p;

  ethernetif = netif->state;
 
  /* move received packet into a new pbuf */
  p = low_level_input(netif);
  /* no packet could be read, silently ignore this */
  if (p == NULL) return;
  /* points to packet payload, which starts with an Ethernet header */
  ethhdr = p->payload;
 
  switch (htons(ethhdr->type)) {

#if ETHARP_TCPIP_ETHINPUT
  /* IP or ARP packet? */
  case ETHTYPE_IP:
  case ETHTYPE_ARP:
    /* full packet send to tcpip_thread to process */
    if (netif->input(p, netif)!=ERR_OK)
     { LWIP_DEBUGF(NETIF_DEBUG, ("ethernetif_init: IP input error\n"));
       pbuf_free(p);
       p = NULL;
     }
    break;                
   
#else /* ETHARP_TCPIP_ETHINPUT */
#if ETHARP_TCPIP_INPUT

  /* IP packet? */
  case ETHTYPE_IP:
#if ETHARP_TRUST_IP_MAC
    /* update ARP table */
    etharp_ip_input(netif, p);
#endif /* ETHARP_TRUST_IP_MAC */
    /* skip Ethernet header */
    pbuf_header(p, -sizeof(struct eth_hdr));
    /* pass to network layer */
    if (netif->input(p, netif)!=ERR_OK)                     /*   should change to "if (netif->input(p, netif)==ERR_MEM)"*/

         { LWIP_DEBUGF(NETIF_DEBUG, ("ethernetif_init: IP input error\n"));
       pbuf_free(p);                                                      /* free again so cause asserting in pbuf.c*/
       p = NULL;
     }
    break;
     
  /* ARP packet? */
  case ETHTYPE_ARP:
    /* pass p to ARP module  */
    etharp_arp_input(netif, ethernetif->ethaddr, p);
    break;
   
#endif /* ETHARP_TCPIP_INPUT */
#endif /* ETHARP_TCPIP_ETHINPUT */

  default:
    pbuf_free(p);
    p = NULL;
    break;
  }
}


FYI:
 
 
Cuihengbin
 
 
 ----------------------------------------------------- 

Cui hengbin <phant>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by phant (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-17 goldsimon StatusNone Fixed
        Open/ClosedOpen Closed
    2007-05-16 fbernon Assigned tofbernon goldsimon
    2007-05-03 fbernon Assigned toNone fbernon

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code