#48 heap_3.c not really thread safe

open
nobody
None
5
2014-08-15
2010-11-17
Bill Howell
No

The example wrapper contained in heap_3.c is still not actually thread-safe, as it only provides protection for the application code that uses the pvPortMalloc and pvPortFree interfaces, not any of the library routines that may call malloc and/or free directly.
Fortunantly there is a solution: the current standard C library has overridable default mallock lock and unlock interfaces, so by properly defining replacements for these, then all calls to malloc and free are made thread-safe.

I have attached a modified version of heap_3.c with these changes as a submission for inclusion in the next release.

Discussion

  • Bill Howell

    Bill Howell - 2010-11-17

    Modified heap_3.c using malloc lock/unlock

     
  • Richard Damon

    Richard Damon - 2010-11-18

    One question is how many of the supported architectures support this method? I will note that the current C standard (C99) does NOT define this function, or in fact, any concept of "thread". (At least it was written so non-supporting ports fall back to the old method).

    I know that when I asked Richard Barry about changing the thread handles from void* to incomplete struct typedefs he mentioned that too many compilers did not support it, and this was in the first ISO standard over 20 years ago.

    Scanning through the FreeRTOS package, there are some ports that do define _malloc_lock/unlock inside their port.c file, so that looks to be the method Richard decided to go with this, and this would allow other ports whose compilers implements other methods of doing this to define their methods.

     
  • Bill Howell

    Bill Howell - 2010-11-18

    Good points, Richard.
    IIRC, __malloc_lock/unlock stubs have been in the GCC libc since somewhere in version 3.x.x, so at least for the GCC ports, (or any other port whose tools are based on GCC) these would be available.

    The lock/unlock stubs are certainly part of the newlib quasi-standard started by Red Hat, which (from their description) was specifically targeted at embedded systems with or without an OS, on which I believe several of the small embedded CPU vendors that sell their code generation tools have based their libc version on, so I would expect many, if not all, of them to have it as well (e.g all of the TI code tools do, as do the PPC and ARM code generators that I have worked with recently).

    Additionally, if a specific port library contains a malloc version that does not define these, then there's no harm done if they are there: the linker won't even include them in the final binary, since they will not be called from anywhere. If anything this may indicate that they should always be included when the underlying memory manager is the libc malloc - although if this path is taken, I'd remove the conditionals within pvPortMalloc/pvPortFree to at least protect the developers use of memory.

    Although my personal focus was on fixing a problem where a library function's use of malloc was intermittently getting tromped on by a call through pvPortMalloc in a different thread, it is also of concern in any asynchronous programming idiom where there are multiple contexts accessing a single system memory pool. The only other fully safe alternate here I can see would be for the programmer to set up individual heaps for each context - essentially implementing per-context dynamic heap management.

    My original thought on how I provided this was that since all of the heap_x.c files were contained within the portable/MemMang directory, that this would be a value added for those ports that supported it.

     
  • Dave

    Dave - 2010-11-18

    While I think this is a valid contribution, maybe to include as heap_4.c, I also agree with richard_damon that many compilers that FreeRTOS uses are unlikely to support this (think of the proprietary Microchip compilers for example). Also Newlib, while targeted at embedded environments, is targeted at different embedded environments than you will normally find FreeRTOS being used in, FreeRTOS being smaller. Many vendors provide their own libraries because Newlib, while smaller than the standard lib, is still large. For example, even in the GCC world (and FreeRTOS is used with a lot more compilers than GCC), CodeRed support NewLib and have their own RedLib too, Rowley supply their own libraries, etc.

    Finally I don't think there has been an attempt or implication in the FreeRTOS world to make libraries thread safe. The FreeRTOS code itself is thread safe, but it can be used with lots of libraries that are not and in those cases mutual exclusion can be maintained by the application itself. pvPortMalloc() is only not thread safe because you are calling malloc() directly, rather than rerouting through pvPortMalloc().

    Just my 2c worth to an interesting debate.

     
  • Bill Howell

    Bill Howell - 2010-11-18

    Hi Dave,

    Appreciate your $(1/50) :)

    I did a bit of looking in the Microchip forums and found that for the Microchip tools:
    1. C18 libraries don't even include malloc/free, so are not applicable to this discussion.
    2. C24, Dsp24 and C32 libraries all include __malloc_lock/unlock in their malloc implementation (someone had posted a zip of their malloc sources in their forums)

    IAR and HiTech are admittedly unknown at this point, as are others who aren't deliberately GCC-based like TI.

    Red Suite 3 source (the 143 Mbyte download is what took me so long to reply) reveals that their malloc also has __malloc_lock and __malloc_unlock.

    It seems the deeper I look, the more the version of malloc implementing the __malloc_lock/unlock stubs turns up as the implementation of choice, at a guess it's most likely because it's been the version included in all of the open source repositories, and lets face it, nobody trying to output a custom IDE or silicon support library for a new whizbang CPU is going to put time and effort into re-inventing the wheel.

    I agree that there can be no guarantee by FreeRTOS to make externally used libraries robustly safe in a multi-threaded environment, but that does beg the question on why there's a heap_3.c that wraps malloc/free calls at all then.

    IMHO this is a simple, efficient method of improving code robustness for what is appearing to be a reasonably large segment of standard C library implementations commonly used.

    Ultimately the issue of code robustness lies on the shoulders of the developers anyway, but there's no reason not to provide them with better tools to do the job. I think that's part of the reason why FreeRTOS exists and is used in the first place, isn't it?

    Regards,
    Bill

     
  • Richard Damon

    Richard Damon - 2010-11-22

    First, for IAR, looking at their files, there is a header file MtxWrapper that defines some routines that define hooks for malloc and company to use to protect the heap in multi-threaded environments.

    I think this just shows that heap_3 is NOT the right place for these definitions, but should be added to the port files.

     
  • Bill Howell

    Bill Howell - 2010-11-24

    Perhaps I have misunderstood the purpose of heap_3.

    I was thinking that because it was kept under the portable directory, that it was intended to be the place to collect the platform differences for providing a malloc wrapper for FreeOS, as well as alternate memory allocators.

    Additionally, my belief that one of it's purposes is to provide a thread-safe malloc implementation is explicitly stated on the FreeRTOS page on Memory Management that heap_3:

    (Below cut and pasted from http://www.freertos.org/a00111.html\):
    Scheme 3 - heap_3.c
    This is just a wrapper for the standard malloc() and free() functions. It makes them thread safe.

    The issue with the current implementation of pvPortMalloc is that it only makes the calls through pvPortMalloc thread safe, any code that calls malloc/realloc/calloc (as is done within the c library itself) bypasses the safeguards there and leaves the heap exposed to potential corruption (as I encountered).

    As far as whether it is the 'proper' place for such interfaces to reside, it seemed to me that keeping them together with the other code providing portable interfaces for thread-safe memory management functions is reasonable, and I have not yet seen an argument presented describing where and how it could present a problem.

     
  • Richard Damon

    Richard Damon - 2010-11-24

    The issue is that since the heap files are in the MemMang and NOT a specific port directory, it contents are expected to be usable by ALL ports. Adding code for a specific group of ports is out of place here. Platform specific files should be under that platforms directory. If (almost) all GCC implementations support this, than maybe a file defining this interface as a MemMang directory under the GCC directory (with maybe copies in the directories for the variations that aren't under the GCC subdirectory). This would put them in the right spot.

    In fact, as I said the first time, at least some of the ports DO provide the __malloc_lock function as you supplied, included in the port.c file.

     

Log in to post a comment.