search

ARMmbed / sal-driver-lwip-k64f-eth

LwIP platform-specific implementation for k64f
Other
1 stars 3 forks source link

malloc used in k64f_low_level_output #3

Closed bremoran closed 8 years ago

bremoran commented 9 years ago

malloc is used in k64f_low_level_output. This could be dangerous if k64f_low_level_output is ever called from an interrupt context. The lwip pool allocator should be used to allocate a new pbuf in this case.

rainierwolfcastle commented 9 years ago

ARM Internal Ref: IOTSFW-981

bogdanm commented 8 years ago

I can't find a reference that says pbuf_alloc is re-entrant. If you know of one, please let me know; otherwise, replacing malloc with pbuf_alloc isn't likely to do a lot of good.

bremoran commented 8 years ago

http://lwip.wikia.com/wiki/Raw/native_API

When running in a multithreaded environment, raw API functions may only be called from the core thread (aka. the tcpip-thread) since raw API functions are not protected from concurrent access (aside from pbuf- and memory management functions)

Note, that does not say "reentrant", it says "protected" which probably means a critical section lock.

If you call pbuf_alloc using the PBUF_POOL type identifier, it will call memp_malloc, which does not appear to be reentrant--it is one atomic op short of being reentrant.

Unfortunately, since our atomic ops are written in C++, it would take some additional effort to port an atomic op for this.

The critical section runs from L410 to L413. https://github.com/ARMmbed/sal-stack-lwip/blob/master/source/lwip/core/memp.c#L410

bogdanm commented 8 years ago

That's what I thought too. I don't think there's an easy fix to this.

bremoran commented 8 years ago

Presumably, we need to add atomic ops to memp_alloc and memp_free.

bremoran commented 8 years ago

Or even disable IRQs/enable IRQs would work. It's a really small critical section.

bogdanm commented 8 years ago

Sure. Unfortunately, even our CriticalSectionLock is in C++ ...