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

[Uwe Bonnes]

Philipp Burch writes:
> 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.
>

Just a short comment: I feel uneager at all that  run-time allocation/ deallocation  happens at all.
 
-- 
Uwe Bonnes                bon at elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------