[En-Nut-Discussion] net/ipout.c memory leak
Stefan Hax
stefan at escherlogic.com
Tue Jun 8 05:50:33 CEST 2010
It true that the current route list has no bounds. It would certainly
be easy to put a limit on the number of entries that may be linked in
the route list, and, easy enough to add aging to a limited list that
would throw out the oldest entry, but, IMO, being able to add a
duplicate route to the linked list is simply a waste of memory.
Stefan.
Henrik Maier wrote:
> 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
>
More information about the En-Nut-Discussion
mailing list