[haiku-commits] Re: haiku: hrev48990 - headers/os/support headers/posix src/system/libroot/posix/malloc_debug

  • From: Paweł Dziepak <pdziepak@xxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sat, 11 Apr 2015 16:33:36 +0200

Firstly, I'm sorry for a very late reply.

2015-04-05 16:06 GMT+02:00 Michael Lotz <mmlr@xxxxxxxx>:

On 04.04.2015 23:35, Paweł Dziepak wrote:

2015-04-04 22:57 GMT+02:00 <mmlr@xxxxxxxx <mailto:mmlr@xxxxxxxx>>:

Commit: b0e31a9ce35e3d3d47b59a21e7c43b0d41c11d11
URL: http://cgit.haiku-os.org/haiku/commit/?id=b0e31a9ce35e
Author: Michael Lotz <mmlr@xxxxxxxx <mailto:mmlr@xxxxxxxx>>
Date: Sat Apr 4 14:38:46 2015 UTC

Revert "malloc_debug: align allocations".

This reverts commit 217f090f9e247d1d4c5644e626642c430fafe4e5.

At least for the guarded heap this completely defeats the purpose. If
software requires a certain alignment it should request it using
memalign explicitly instead of assuming it.

This is not true. malloc() and friends have to return buffers aligned to
alignof(max_align_t) (except, obviously, buffers smaller than
sizeof(max_align_t)), so software can assume that returned memory is
appropriately aligned (well, it would be insanely stupid if, e.g., you
wouldn't be able to store a long double in a buffer returned by
malloc(sizeof(long double))). All that also means that on x86[_64]
dynamically allocated memory can also be used to store __m128 types
without worrying about alignment.


Thanks for pointing out std::max_align_t. I see now that this was
standardized in C++11. The commit that introduced this unfortunately only
mentioned that "the software assumes it" and just used 8 without further
explanation which sounded more like a bug.


Well, it's no just a requirement added with C++11. The standard merely
formalized the already existing alignment guarantees and requirements
(meaning: this applies also to gcc2). For example, x86 atomic stores and
loads aren't really atomic if the memory location isn't naturally aligned.



Revision: hrev48990
Commit: 121655e9ee3e7fa6d9244df8c68ad30f9981af8c
URL: http://cgit.haiku-os.org/haiku/commit/?id=121655e9ee3e
Author: Michael Lotz <mmlr@xxxxxxxx <mailto:mmlr@xxxxxxxx>>
Date: Sat Apr 4 14:41:45 2015 UTC

malloc_debug: Add default alignment option.

This allows for something similar as was implemented in 217f090 but
makes it optional and configurable.

The MALLOC_DEBUG environment variable now can take "a<size>" to set
the default alignment to the specified size. Note that not all
alignments may be supported depending on the heap implementation.


Could you elaborate on why is this patch necessary? I fail to see any
reason why one would want to have the default alignment configurable at
runtime.


The point of the guarded heap is to ensure that the buffer returned ends
on a page boundary where the page following the allocation is read and
write protected, hence crashing the application on out of bound
reads/writes. It helps to make buffer overflows and use-after-free obvious,
even in cases where you might normally not notice due to memory not being
overwritten quickly enough or where it is likely to not produce content
that would otherwise lead to a crash.

When using this feature it is obviously undesirable to have alignment
where non is strictly needed. With an alignment of 8 you will hide out of
bound access of up to 7 bytes depending on allocation size.

The option with the default alignment therefore allows the use with
software that requires such an alignment while not making the feature less
useful for cases where it isn't needed.


That looks more like an inherent limitation of the guarded heap
implementation rather than something that can be fixed by violating
alignment requirements. Writing some data to the unprotected bytes after
the end of the buffer and then verifying that they weren't changes may be
some way of trying to alleviate that issue (obviously the disadvantage is
that we won't get abort exactly at the moment incorrect write is performed,
and it won't work for reads). Anyway, currently the best way to verify
memory accesses seems to be using compiler instrumentation as
AddressSanitizer undoubtedly makes any paging-based approaches obsolete.



In light of std::max_align_t the option could simply be reversed, i.e.
having the default alignment set to std::max_align_t where it is available
and allowing setting it back to 1 using that option.


Setting default alignment to anything smaller than the alignment required
by the ABI is simply wrong (e.g. atomics will break) so I don't really see
any reason why it should be a run-time option instead of compile-time
constant: it cannot be smaller than std::max_align_t and there is no point
in setting default alignment that is more strict.



Note that this is a debug feature that has to explicitly be used by
someone debugging software (using LD_PRELOAD=libroot_debug.so and in the
case of the userland guarded heap by compiling libroot_debug with it). It
does not affect the normal heap that is used in production and debug builds.


The fact that it is only a debug tool doesn't mean that the code quality
isn't important anymore. ;)



Regards,
Michael


Other related posts:

  1. Home
  2. haiku-commits
  3. Archive
  4. 04-2015