Memory leaks and other issues reported by valgrind

[appscrunch]

Memory leaks and other issues reported by valgrind

[Enmk]

Thanks for contributing!
Let me quickly go through your report.

memcpy

There are several reported mis-usages of memcpy by libsecp256k1, which, IMO could be a problem.

Invalid reads

Well, all that happens at Object::get_magic(), and this behavior is intentional. Sole purpose of this is to either:

• Detect that object being passed by the clients is of wrong type.
• Object was already deleted/freed.

In both cases we could crash anyway some time later, either by writing to random valid memory location (messing up user data and causing havoc) or by writing/reading from invalid memory location (read: unloaded page) and hitting SIGSEG.

By doing that validity check we are trying to detect and recover or crash soon, before any mission-critical user data is lost.

What you can do

You can re-run valgrind for parent process only with --trace-children=yes or you can try disable or filter-out death tests.

Memory leaks

There are two sources of memory leaks in report:

• death-tests - all leaks come from forked process (e.g. PID is 17017, while parent is 17013) that is supposed to die, dies fast and has no chance of freeing any of its resources.
• The libsecp256k1 context not being freed. That context is allocated only once for application and never freed. We might re-consider that during post-beta refactoring.

Uninitialised values

There are quite a few of those:

• Use of uninitialised value of size 8
• Conditional jump or move depends on uninitialised value(s)
  Some of those are clearly bogus, like this one:

==17013== Conditional jump or move depends on uninitialised value(s)
==17013==    at 0x4C30F78: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17013==    by 0x516B5BD: multy_core::internal::copy_string(char const*) (utility.cpp:92)
==17013==    by 0x516B56C: multy_core::internal::copy_string(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (utility.cpp:82)
==17013==    by 0x515A50E: key_to_string (key.cpp:140)

while others require more investigation.

Conclusion

Thank you for your effort, there are some really interesting findings. As a side note, I think that we should add a valgrind-run to our test suite once CI is ready.

[tokutech]

Cool, as a library it should be ok to valgrind.
But valgrind is slow, we can use AddressSanitizer to fast check(who is supportted in GCC&Clang now):
https://clang.llvm.org/docs/AddressSanitizer.html

Here is a demo from mysql CMakeLists.txt:
https://github.com/percona/percona-server/blob/5.7/CMakeLists.txt#L308

[Enmk]

No activity for a long time, closing.