On Mon, Jun 03 2019, Stuart Henderson <[email protected]> wrote: > 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 was thinking of freezero(3) as an alternative to sodium_memzero(), not as a solution for the lack of malloc_usable_size(3). I should have made this clearer, or not mention freezero(3) at all, sorry about that. > 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. IIUC their use of -fsized-deallocation means that delete(ptr, size) will be preferred by the compiler when possible, ie when the size of the object is known. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3778.html So while delete(ptr) can't be properly implemented as intended by upstream as long as we don't have malloc_usable_size(3), OpenBSD users can still benefit from (parts of) the improvements in https://github.com/keepassxreboot/keepassxc/pull/3020 In my opinion the code in Alloc.cpp should be kept and made portable as suggested, preferably by someone who uses keepassxc and knows his way in cmake and C++ land, ie not me. ;) Bonus: keepassxc would build without patches on OpenBSD. [...] -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
