On 2019/06/02 17:41, Jeremie Courreges-Anglas wrote:
> > Index: patches/patch-src_core_Alloc_cpp
> > ===================================================================
> > RCS file: patches/patch-src_core_Alloc_cpp
> > diff -N patches/patch-src_core_Alloc_cpp
> > --- /dev/null 1 Jan 1970 00:00:00 -0000
> > +++ patches/patch-src_core_Alloc_cpp 2 Jun 2019 09:44:06 -0000
> > @@ -0,0 +1,23 @@
> > +$OpenBSD$
> > +
> > +Index: src/core/Alloc.cpp
> > +--- src/core/Alloc.cpp.orig
> > ++++ src/core/Alloc.cpp
> > +@@ -20,6 +20,8 @@
> > + #include <sodium.h>
> > + #ifdef Q_OS_MACOS
> > + #include <malloc/malloc.h>
> > ++#elif defined(Q_OS_OPENBSD)
> > ++#include <stdlib.h>
> > + #else
> > + #include <malloc.h>
> > + #endif
> > +@@ -61,7 +63,7 @@ void operator delete(void* ptr) noexcept
> > + ::operator delete(ptr, _msize(ptr));
> > + #elif defined(Q_OS_MACOS)
> > + ::operator delete(ptr, malloc_size(ptr));
> > +-#elif defined(Q_OS_UNIX)
> > ++#elif defined(Q_OS_LINUX)
> > + ::operator delete(ptr, malloc_usable_size(ptr));
> > + #else
> > + // whatever OS this is, give up and simply free stuff
> >
>
> I think the port's cmake setup should check for malloc_usable_size and
> malloc.h, use them if available, and fall back on stdlib.h / std::free()
> if not. OpenBSD is not the only OS where malloc.h isn't available, and
> Linux (glibc really) isn't the only OS where malloc_usable_size() is
> available. This kind of #ifdef practice is actively harmful for
> portability.
+1
> Also if the goal is to securely erase memory before freeing it,
> maybe freezero(3) is a possible alternative?
The upstream commit does a couple of things.
One part is overriding the delete operator as a best-effort way to try to
wipe all their c++ dynamic allocations. They're using malloc_usable_size()
to identify how much memory should be zeroed. In their case they use
sodium_memzero() to do this, but freezero() would be pretty much equivalent,
and neither would work for them: for both you need to know how much
memory should be zeroed at the time you want to free it.
I'm not sure there's much that can be done about that without major changes
to track allocations which are well out of scope for doing in ports patches.
The other part is that they're using libgcrypt's alloc_secure_func for some
pieces which they know are keys ("As a further improvement, this patch
uses libgcrypt and libsodium to write long-lived master key component
hashes into a secure memory area and wipe it afterwards.") - seems like
malloc_conceal would be perfect for this, probably in gcrypt itself
rather than programs using gcrypt, though I soon got lost in the pools
stuff in gcrypt src/secmem.c when trying to figure out how to do it.
