[En-Nut-Discussion] net/ipout.c memory leak

[Stefan Hax]

Found the leak in the form of duplicate entries in the routing table.  A 
simple fix I propose as follows in net/route.c :

int NutIpRouteAdd(uint32_t ip, uint32_t mask, uint32_t gate, NUTDEVICE * 
dev)
{
     int rc = -1;
     RTENTRY *rte;
     RTENTRY *rtp;
     RTENTRY **rtpp;

...added code......
     rtp = rteList;

     while (rtp) {               // go through the whole list!!
                                 // check for duplicate!!
         if( (rtp->rt_ip == (ip & mask)) && (rtp->rt_mask == mask) && 
(rtp->rt_gateway == gate) && (rtp->rt_dev == dev) ) {
             return 0;           // duplicate found ... bye
         }
     rtp = rtp->rt_next;
     }
..................

     /*
      * Insert a new entry into the list, which is
      * sorted based on the mask. Host routes are
      * in front, default routes at the end and
      * networks in between.
      */
     rtp = rteList;
     rtpp = &rteList;
     while (rtp && rtp->rt_mask > mask) {
...etc...

Stefan.

Stefan Hax wrote:
> Sorry, the call to (*nif->if_output)() already frees nb_clone in case of 
> error.   Although I have not found my memory leak, NutNetBufFree() does 
> not look right when dealing with clones. Here is my suggestion:
> 
> extending nb_flags in the structure to be a uint16_t instead of a 
> uint8_t then:
> #define NBAF_DIE	0x0100
> 
> ... in /net/netbuf.c:
> 
> void NutNetBufFree(NETBUF * nb)
> {
>      if (nb->nb_flags & NBAF_REFCNT) {                   // referecned 
> by something??
>          nb->nb_flags |= NBAF_DIE;                       // just mark 
> this for death
> 
>      } else {                                            // no ref... 
> start free'ing what is owned
>          if (nb->nb_flags & NBAF_DATALINK) {
>              if(nb->nb_ref) {
>                  if(nb->nb_ref->nb_dl.vp != nb->nb_dl.vp) { 
>   // our's to relese?? then relese it.
>                      NutHeapFree(nb->nb_dl.vp);
>                  }
>              } else {
>                  NutHeapFree(nb->nb_dl.vp);
>              }
>          }
> 
>          if (nb->nb_flags & NBAF_NETWORK) {
>              if(nb->nb_ref) {
>                  if(nb->nb_ref->nb_nw.vp != nb->nb_nw.vp) { 
>   // our's to relese?? then relese it.
>                      NutHeapFree(nb->nb_nw.vp);
>                  }
>              } else {
>                  NutHeapFree(nb->nb_nw.vp);
>              }
>          }
> 
>          if (nb->nb_flags & NBAF_TRANSPORT) {
>              if(nb->nb_ref) {
>                  if(nb->nb_ref->nb_tp.vp != nb->nb_tp.vp) { 
>   // our's to relese?? then relese it.
>                      NutHeapFree(nb->nb_tp.vp);
>                  }
>              } else {
>                  NutHeapFree(nb->nb_tp.vp);
>              }
>          }
> 
>          if (nb->nb_flags & NBAF_APPLICATION) {
>              if(nb->nb_ref) {
>                  if(nb->nb_ref->nb_ap.vp != nb->nb_ap.vp) { 
>   // our,s to relese?? then relese it.
>                      NutHeapFree(nb->nb_ap.vp);
>                  }
>              } else {
>                  NutHeapFree(nb->nb_ap.vp);
>              }
>          }
> 
>          if (nb->nb_ref) {
>              if(nb->nb_ref->nb_flags & NBAF_REFCNT) {            // dec 
> parent count of this ref??
>                  nb->nb_ref->nb_flags--;
>              }
> 
>              if(nb->nb_ref->nb_flags & NBAF_DIE) {               // 
> parent marked for death?? then free it..
>                  NutNetBufFree(nb->nb_ref);
>              }
>          }
> 
>          NutHeapFree(nb);
>      }
> }
> 
> If what is being freed is the 'parent' and their are still references to 
> it then only mark for freeing when all clones have been freed. 
> Otherwise, this will just release what bufferes were allocated when the 
> clone was created and decrement the reference count in the parent then 
> free the clone, and finally, the parent is freed once all references are 
> gone.
> 
> Stefan.
> 
> Stefan Hax wrote:
>> noticed a small memory leak in ../net/ipout.c during UDP broadcast failures.
>>
>> code is:
>> NutIpOutput(..)
>> {
>> ...
>>                 if (nif->if_type == IFT_ETHER)
>>                      rc = (*nif->if_output) (dev, ETHERTYPE_IP, ha, 
>> nb_clone);
>>                  else if (nif->if_type == IFT_PPP)
>>                      rc = (*nif->if_output) (dev, PPP_IP, 0, nb_clone);
>>
>>                  if (rc == 0) {
>>                      NutNetBufFree(nb_clone);
>>                  }
>> ...
>> }
>>
>>
>> sugested:
>> NutIpOutput(..)
>> {
>> ...
>>                 if (nif->if_type == IFT_ETHER)
>>                      rc = (*nif->if_output) (dev, ETHERTYPE_IP, ha, 
>> nb_clone);
>>                  else if (nif->if_type == IFT_PPP)
>>                      rc = (*nif->if_output) (dev, PPP_IP, 0, nb_clone);
>>
>>                  if(nb_clone) {          /* <-- might be a better choice */
>>                      NutNetBufFree(nb_clone);
>>                  }
>> ...
>> }
>>
>>
>> Regards, Stefan.
>> _______________________________________________
>> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
>>
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
>