r6796 introduced possible memory leak in os/timer.c

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

r6796 introduced possible memory leak in os/timer.c

Philipp Burch-2
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
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: 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                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: r6796 introduced possible memory leak in os/timer.c

Philipp Burch-2
Hi Uwe!

On 16.01.21 19:54, Uwe Bonnes wrote:
>
> Just a short comment: I feel uneager at all that  run-time allocation/ deallocation  happens at all.
>  
>

Indeed. Especially that timers need heap memory in all kinds of OS
sleeps is not really nice. With the implemented NutTimerHandleStart(),
it would probably be possible to use a single pre-allocated timer
structure per thread. But I don't really feel comfortable with hacking
in the innermost parts of Ethernut.

Considering the change done in r6796: Do you know why tn_flags was
implemented as a uint32_t? Since Ethernut also targets 8-bit platforms,
it seems somewhat inefficient to add a 32-bit value for a single bit to
one of the core OS structures.

Regards,
Philipp
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: r6796 introduced possible memory leak in os/timer.c

Uwe Bonnes
Philipp Burch writes:

> Hi Uwe!
>
> On 16.01.21 19:54, Uwe Bonnes wrote:
> >
> > Just a short comment: I feel uneager at all that  run-time allocation/ deallocation  happens at all.
> >  
> >
>
> Indeed. Especially that timers need heap memory in all kinds of OS
> sleeps is not really nice. With the implemented NutTimerHandleStart(),
> it would probably be possible to use a single pre-allocated timer
> structure per thread. But I don't really feel comfortable with hacking
> in the innermost parts of Ethernut.

I neither. But I wanted a possibility to get rid of the runtime heap
de/allocation.

>
> Considering the change done in r6796: Do you know why tn_flags was
> implemented as a uint32_t? Since Ethernut also targets 8-bit platforms,
> it seems somewhat inefficient to add a 32-bit value for a single bit to
> one of the core OS structures.
>
You missed the " by you" in "Do you know why tn_flags was implemented as
a uint32_t" ;-)

I did not think of our poor memory starving AVR8
architecture. Changing flags to uint8_r should do no harm (famous last
words).

Where did you apply 6917 and 6918. "git svn rebase" only gives me Ole's
change for 6916. Please either apply yourself to master or tell me to
do so.

Bye

--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: r6796 introduced possible memory leak in os/timer.c

Philipp Burch-2
Hi Uwe!

On 17.01.21 21:50, Uwe Bonnes wrote:
> I did not think of our poor memory starving AVR8
> architecture. Changing flags to uint8_r should do no harm (famous last
> words).

It would certainly be required to also change the #define to 0x80 or
something that fits in 8 bits. It could then be put to the other flag
(TM_ONESHOT), so that it doesn't get missed in future updates.

>
> Where did you apply 6917 and 6918. "git svn rebase" only gives me Ole's
> change for 6916. Please either apply yourself to master or tell me to
> do so.

I still keep the Ethernut sources in a pure Subversion repo, didn't make
the (long due) change to Git yet.

The changes are in the devnut_tiva branch:

https://sourceforge.net/p/ethernut/code/HEAD/tree/branches/devnut_tiva/

I don't know how the Subversion-Branches are handled by Git, if they are
in the repo at all.

Best regards,
Philipp
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: r6796 introduced possible memory leak in os/timer.c

Thiago A. Corrêa
If you have permission to commit to branches you should be able to
commit to the trunk.

By the way, maybe the repository should be moved to github for better
visibility, easier for users to contribute.

On Mon, Jan 18, 2021 at 2:39 PM Philipp Burch <[hidden email]> wrote:

>
> Hi Uwe!
>
> On 17.01.21 21:50, Uwe Bonnes wrote:
> > I did not think of our poor memory starving AVR8
> > architecture. Changing flags to uint8_r should do no harm (famous last
> > words).
>
> It would certainly be required to also change the #define to 0x80 or
> something that fits in 8 bits. It could then be put to the other flag
> (TM_ONESHOT), so that it doesn't get missed in future updates.
>
> >
> > Where did you apply 6917 and 6918. "git svn rebase" only gives me Ole's
> > change for 6916. Please either apply yourself to master or tell me to
> > do so.
>
> I still keep the Ethernut sources in a pure Subversion repo, didn't make
> the (long due) change to Git yet.
>
> The changes are in the devnut_tiva branch:
>
> https://sourceforge.net/p/ethernut/code/HEAD/tree/branches/devnut_tiva/
>
> I don't know how the Subversion-Branches are handled by Git, if they are
> in the repo at all.
>
> Best regards,
> Philipp
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Does tiva still have that stupid header license?

Uwe Bonnes
In reply to this post by Philipp Burch-2
Hello,

did TI get to their senses and put the tiva header under a sensible license at some point?
I'd like to merge devnut-tiva.


Cheers
--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: r6796 introduced possible memory leak in os/timer.c

Uwe Bonnes
In reply to this post by Thiago A. Corrêa
Thiago A. Corrêa writes:
> If you have permission to commit to branches you should be able to
> commit to the trunk.
>
> By the way, maybe the repository should be moved to github for better
> visibility, easier for users to contribute.
>
I work all the time with git svn and am comfordable with that. For the
tiva changes :
- git checkout devnut_tiva
- git rebase origin/devnut_tiva got that branch up to date.

Changing fully to github is some major effort and will look out some
old-time users from their access. Perhaps a  mirror on github?

(Other) Opinions?

Bye
--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: Does tiva still have that stupid header license?

Philipp Burch-2
In reply to this post by Uwe Bonnes
Hi Uwe!

On 18.01.21 19:14, Uwe Bonnes wrote:
> Hello,
>
> did TI get to their senses and put the tiva header under a sensible license at some point?
> I'd like to merge devnut-tiva.

I was a bit disappointed when I saw that there are still no headers for
download, just their huge Windows executable containing the whole
software stuff:

https://www.ti.com/tool/download/SW-TM4C

But TI obviously changed something. There are apparently no more CMSIS
headers included, but a folder of header files with part and component
definitions, starting with this kind of license:


//*****************************************************************************
//
// tm4c1294ncpdt.h - TM4C1294NCPDT Register Definitions
//
// Copyright (c) 2013-2020 Texas Instruments Incorporated.  All rights
reserved.
// Software License Agreement
//
//   Redistribution and use in source and binary forms, with or without
//   modification, are permitted provided that the following conditions
//   are met:
//
//   Redistributions of source code must retain the above copyright
//   notice, this list of conditions and the following disclaimer.
//
//   Redistributions in binary form must reproduce the above copyright
//   notice, this list of conditions and the following disclaimer in the
//   documentation and/or other materials provided with the
//   distribution.
//
//   Neither the name of Texas Instruments Incorporated nor the names of
//   its contributors may be used to endorse or promote products derived
//   from this software without specific prior written permission.
//
// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
//
// This is part of revision 2.2.0.295 of the Tiva Firmware Development
Package.
//
//*****************************************************************************


Looks like we've been heard :) The toplevel directory even includes a
file named TI-BSD-EULA.txt with this content. So assuming that TI does
not decide to obsolete the whole series during the next months, it could
actually be beneficial for others to finally merge the devnut_tiva
branch. But I should first integrate those new headers and check if the
stuff still works...

Bye,
Philipp
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Git x SVN (Re: r6796 introduced possible memory leak in os/timer.c)

Dušan-2
In reply to this post by Uwe Bonnes
Hi guys,

there was a discussion about git vs svn several years ago
and I think Harald pointed out several reasons, why to stay with svn.
Maybe one of them was the sourceforge repository.

 From that time, github improved and I feel more comfortable with the
github UI, especially for task tracking.

---------
We placed our Kinetis port at https://github.com/dferbas/ethernut.
Nowadays we have a running application port from MCF52259 on the
MK64FN1M0VLQ12.
These days we are working on a smaller board with MK60DN256VLL10,
pin compatible with MK60DN512VLL10 and MK64FX512VLL12, MK64FN1M0VLL12 .

This time, we left the original Nut nearly unchanged.
Our previous port for Coldfire cpus used the "NutUseCritical()", which
was not accepted by the community
and we never put an effort to run a long test without preserving
interrupt level in application threads.
The previous port is also on github: https://github.com/dferbas/NutOs.

Once we have the ppp working (this is where we differ a lot), I will
think about merging into Nut head.

*Dusan*

On 18.1.2021 19:20, Uwe Bonnes wrote:

> Thiago A. Corrêa writes:
>> If you have permission to commit to branches you should be able to
>> commit to the trunk.
>>
>> By the way, maybe the repository should be moved to github for better
>> visibility, easier for users to contribute.
>>
> I work all the time with git svn and am comfordable with that. For the
> tiva changes :
> - git checkout devnut_tiva
> - git rebase origin/devnut_tiva got that branch up to date.
>
> Changing fully to github is some major effort and will look out some
> old-time users from their access. Perhaps a  mirror on github?
>
> (Other) Opinions?
>
> Bye
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: Git x SVN (Re: r6796 introduced possible memory leak in os/timer.c)

Thiago A. Corrêa
    Yes, Github now has much better visibility and the Pull Request
mechanism allows easier contributions and code reviews.
    There are much nicer git GUI programs now such as fork.dev and
many IDEs now have it too.

    I migrated mercurial repositories to github, they have a very good
importer that keeps the history of the commits.


On Tue, Jan 19, 2021 at 8:36 PM Dušan <[hidden email]> wrote:

>
> Hi guys,
>
> there was a discussion about git vs svn several years ago
> and I think Harald pointed out several reasons, why to stay with svn.
> Maybe one of them was the sourceforge repository.
>
>  From that time, github improved and I feel more comfortable with the
> github UI, especially for task tracking.
>
> ---------
> We placed our Kinetis port at https://github.com/dferbas/ethernut.
> Nowadays we have a running application port from MCF52259 on the
> MK64FN1M0VLQ12.
> These days we are working on a smaller board with MK60DN256VLL10,
> pin compatible with MK60DN512VLL10 and MK64FX512VLL12, MK64FN1M0VLL12 .
>
> This time, we left the original Nut nearly unchanged.
> Our previous port for Coldfire cpus used the "NutUseCritical()", which
> was not accepted by the community
> and we never put an effort to run a long test without preserving
> interrupt level in application threads.
> The previous port is also on github: https://github.com/dferbas/NutOs.
>
> Once we have the ppp working (this is where we differ a lot), I will
> think about merging into Nut head.
>
> *Dusan*
>
> On 18.1.2021 19:20, Uwe Bonnes wrote:
> > Thiago A. Corrêa writes:
> >> If you have permission to commit to branches you should be able to
> >> commit to the trunk.
> >>
> >> By the way, maybe the repository should be moved to github for better
> >> visibility, easier for users to contribute.
> >>
> > I work all the time with git svn and am comfordable with that. For the
> > tiva changes :
> > - git checkout devnut_tiva
> > - git rebase origin/devnut_tiva got that branch up to date.
> >
> > Changing fully to github is some major effort and will look out some
> > old-time users from their access. Perhaps a  mirror on github?
> >
> > (Other) Opinions?
> >
> > Bye
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion