[En-Nut-Discussion] r6796 introduced possible memory leak in os/timer.c

[Philipp Burch]

Hi everyone,

I work with the devnut_tiva branch of Ethernut which I update 
irregularly with the changes from trunk. Since such a merge in last 
september, I started to experience spurious memory leaks in one of our 
applications. The symptom is that the available heap memory continuously 
decreased, sometimes faster, sometimes slower, until the whole 
application finally crashed. Since our software changed a lot in the 
meantime, I first searched there for the reason, but without success. So 
I finally reverted the Ethernut sources back to before the merge (to 
r6786, almost two years back) and the issue was gone.

While running through all the changes introduced between r6786 and 
r6912, the following change of r6796 in os/timer.c caught my attention:

-            if ((tn->tn_ticks_left = tn->tn_ticks) == 0) {
-                NutHeapFree(tn);
-            }
-            else {
+            tn->tn_ticks_left = tn->tn_ticks;
+            if (tn->tn_ticks_left == 0) {
+                if (!(tn->tn_flags & TM_HAS_OWN_STRUCT)) {
+                    NutHeapFree(tn);
+                }
+            } else {

Reverting just this change (by commenting out the check for 
TM_HAS_OWN_STRUCT) also fixed the behaviour. But the change as such is 
fine and not the actual error.

The error is in NutTimerCreate(), a few lines further down:

static NUTTIMERINFO * NutTimerCreate(uint32_t ticks, void (*callback) 
(HANDLE, void *), void *arg, uint8_t flags)
{
     NUTTIMERINFO *tn;

     tn = NutHeapAlloc(sizeof(NUTTIMERINFO));
     if (tn) {
         tn->tn_ticks_left = ticks + NutGetTickCount() - nut_ticks_resume;

         /*
          * Periodic timers will reload the tick counter on each timer
          * intervall.
          */
         if (flags & TM_ONESHOT) {
             tn->tn_ticks = 0;
         } else {
             tn->tn_ticks = ticks;
         }

         /* Set callback and callback argument. */
         tn->tn_callback = callback;
         tn->tn_arg = arg;
     }
     return tn;
}

The function checks the passed flags argument, but fails to write it 
into the newly created tn->tn_flags. This means that the tn_flags field 
stays uninitialized and whatever the memory contents were before being 
allocated with NutHeapAlloc() is now set for this field. So if, by 
coincidence, the memory contained 0x80000000, the timer will have 
TM_HAS_OWN_STRUCT set, causing the memory leak because the structure 
will not be freed when it expires.

The fix is trivial:

--- nut/os/timer.c	(revision 6917)
+++ nut/os/timer.c	(working copy)
@@ -641,6 +641,7 @@
          /* Set callback and callback argument. */
          tn->tn_callback = callback;
          tn->tn_arg = arg;
+        tn->tn_flags = flags;
      }
      return tn;
  }

I committed it as r6918, but only for the devnut_tiva branch; I think I 
don't even have permission to commit to trunk. I assume that it is a 
good idea to use the flags given to NutTimerCreate() to avoid confusion, 
but it would be just as fine to properly initialize tn_flags to zero.

Best regards,
Philipp