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

[Henrik Maier]

I remember also having problems with duplicate route table entries a few
years ago after reconfiguring the network during runtime. This is of course
no issue on startup, only if network is reconfigured at a later stage.

I worked around this by always clearing the whole table before calling any
of the IfConfig routines like this:
 
   NutIpRouteDelAll(...);
   NutDhcpIfConfig(...); or NutNetIfConfig2 (...);

To change NutIpRouteAdd to avoid duplicate entries won't fix another issue
that the route table can grow without limit. At some stage the system will
run out of memory. For that reason I also suggest to always clear the route
table when the network is reconfigured and include NutIpRouteDelAll in the
NutNetConfig() calls.

Henrik

> -----Original Message-----
> From: en-nut-discussion-bounces at egnite.de [mailto:en-nut-discussion-
> bounces at egnite.de] On Behalf Of Stefan Hax
> Sent: Tuesday, 8 June 2010 2:36 AM
> To: Ethernut User Chat (English)
> Subject: Re: [En-Nut-Discussion] net/ipout.c memory leak
> 
> 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
> >
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion