Every memory allocation has to be checked

[fjullien]

During my evaluation of picoTCP I had several problems due to memory allocation.

As my target is short on memory, it is frequent that a malloc fails and sometimes it can be hard to track without proper check and report in the stack base code.

For example, I had problems with fragments. The root cause was in pico_ipv4_process_frag: https://github.com/tass-belgium/picotcp/blob/master/modules/pico_fragments.c#L382

pico_tree_insert(&ipv4_fragments, pico_frame_copy(f));

Here pico_frame_copy is not tested. In my case it returned NULL...

There is a lot of places where dynamically allocated memory is not tested. It should be fix.

Franck.

[davidcl]

An easy way to track these issues is to setup a (free) Coverity Scan project or to run any other static analyzer. Is there any plan on that ?

[Q-Thomas]

@fjullien : Thank you for reporting this issue. (And the other issues too, thanks for taking the time and effort!) Currently we are a bit limited on manpower due to the holiday period over here, but next week our lead developer will be back in business and I will make sure he checks this (and your other issues) out.

@davidcl : We have Coverity Scan running on our Development branch, you can check it our here. I just added the link and badge to it to our README. We also have a tool called TICS running on both our development and release branches, for which you can check the results out here. But thank you for the suggestion!

[frederikvs]

Apologies that it took me this long to respond - it's been a busy week.

@fjullien You're quite right about the pico_tree_insert needing to be checked, we already had an issue tracking that (#380), and already had some work done in that direction, but it wasn't completely satisfactory yet. Right now we have a little more bandwidth than in recent weeks, so I've asked somebody to take care of that issue.

However, your remarks here are more general than just the tree_insert issue, you're talking about "Every memory allocation".

Things we already do to check for memory leaks :

• We have some ways of checking correct handling of malloc failures in unit tests (a special malloc, that can be set to fail). Unfortunately, this only works when the tester uses it.
• We also run it under valgrind ASAN, but of course, that only catches stuff that actually occurs in that run. I don't think we run out of memory all that often, so leaks coming from a memory shortage don't get caught.
• We already had a coverity scan project set up a while back, but we hadn't had the time to properly process the output. Recently we did get a chance to do that, and I'm told that in our development branch we're "clean" now.

Since none of these reported the problem you mentioned, they're obviously not yet satisfactory. If you (or anyone else) have other suggestions on how to go about this, I'm all ears :-)

edit : I mentioned we run under valgrind, but it's actually ASAN we use.

[frederikvs]

Just had a short brainstorm with @ISY-thomas, and we came up with the idea of having a malloc that randomly fails, and running an existing set of tests on that, with ASAN to catch the problems. I opened another issue for that (#403), since it's quite a specific task, and I'd like to keep this issue for further ideas.

If anybody else can come up with something new, don't hesitate :-)

[fjullien]

I just greped the sources for pico_frame_copy and found other potential issues. For example:

https://github.com/tass-belgium/picotcp/blob/master/modules/pico_arp.c#L228 https://github.com/tass-belgium/picotcp/blob/master/modules/pico_ipv6_nd.c#L952 ....

Another example: pico_socket_sendto_destination can return NULL. However, it is not tested here: https://github.com/tass-belgium/picotcp/blob/master/stack/pico_socket.c#L1337

It took me 5 minutes to find those so I think a static, handmade analysis can also be a good start !