MySQL Bugs: #115120: Provide memory debugging macro implementations for AddressSanitizer

Bug #115120	Provide memory debugging macro implementations for AddressSanitizer
Submitted:	24 May 2024 16:25	Modified:	16 Jan 10:08
Reporter:	Laurynas Biveinis (OCA)	Email Updates:
Status:	Won't fix	Impact on me:	None
Category:	MySQL Server: Compiling	Severity:	S4 (Feature request)
Version:	8.4.0.9.0	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	Contribution

Description:
include/memory_debugging.h defines several macros to help with heap debugging: MEM_MALLOCLIKE_BLOCK, MEM_FREELIKE_BLOCK, MEM_UNDEFINED, MEM_NOACCESS, MEM_CHECK_ADDRESSABLE, etc. These macros have implementations for Valgrind.

AddressSanitizer has user request facility too. It does not map to Valgrind ones exactly but I believe it is possible to do something meaningful for all of the above macros.

One macro which does not map nicely to ASan user requests is MEM_DEFINED_IF_ADDRESSABLE. Luckily, it is also never used, so it is an option to simply remove it.

How to repeat:
See above

Suggested fix:
Will attach. The testsuite completes with no regression with the patch and ASan enabled

Bug 115120 patch for 8.4.0 (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: bug115120-8.4.0.patch (application/octet-stream, text), 2.60 KiB.

Hello Laurynas,

Thank you for the report and contribution.

regards,
Umesh

My contributed patch applies cleanly on 9.0.0 and passes the tests.

The contributed patch applies on 9.1.0 cleanly and passes the tests.

Posted by developer:
 
Thank you very much for your contribution and long-time interest in MySQL.

Unfortunately, we have decided not to include it.

While we agree that making use of external tools, like ASAN, to also check
internal memory management facilities is an excellent idea, we do not favor the
chosen approach of redefining the exiting MEM_ macros to call
ASAN_POISON_MEMORY_REGION and ASAN_UNPOISON_MEMORY_REGION.

This is because the MEM_ macros are modeled after the corresponding Valgrind
macros, and used in the code base with assumtion that they have same semantics
as the Valgrind ones, and these semantics cannot be re-created with ASAN.

In the case of MEM_MALLOCLIKE_BLOCK and MEM_FREELIKE_BLOCK these macros are
used when calling the system malloc and free fuctions which ASAN already
intercepts and adds suitable poisoning unpoisoning for. And the ASAN
documentation explicitly cautions against using manual poisoning when that is
not needed:

"A user may poison/unpoison a region of memory manually. Use
this feature with caution. In many cases good old malloc+free
is a better way to find heap bugs than using custom allocators
with manual poisoning."
(https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning)

The contribution suggests that MEM_NOACCESS should map to
ASAN_POISON_MEMORY_REGION and MEM_UNDEFINED map to ASAN_UNPOISON_MEMORY_REGION.
While it may seem reasonable to implement MEM_NOACCESS with poisoning, it would
be very confusing to implement MEM_UNDEFINED with ASAN_UNPOISON_MEMORY_REGION.
Notably because the TRASH macro uses MEM_UNDEFINDED, and TRASH is used on
accessible memory which is not poisoned. Moreover, unlike
VALGRIND_MAKE_MEM_UNDEFINED, ASAN_UNPOISON_MEMORY_REGION is a noop when called
on unpoisoned memory, and will not trigger any error/warning if that memory is
read before it has been written, which is the main reason for including it in
the TRASH macro.  Letting MEM_NOACCESS map to ASAN_POISON_MEMORY_REGION without
a corresponding macro to do unpoisoning does not seem like a good solution.

Note that in storage/temptable/include/temptable/block.h MEM_NOACCESS and
MEM_UNDEFINDED ARE used in a way that fits with poison/unpoison mapping
suggested in the contribution, so it may be possible to use ASAN manaual
poisoning here, but it seems more appropriate to create a separate facility
(separate macros) for this (or just call the ASAN macros directly with suitable
HAVE_ASAN checks).

When it comes MEM_CHECK_ADDRESSABLE() the proposed ASAN implementation of just
asserting that the memory is not poisoned seems harmless. But it is only used
in the TRASH macro (and a similar facility in ndb).  Which is very unlikely to
be called on poisoned memory, so it is somewhat doubtful that this will catch
many errors.

Finally, note that a genuine use case for manual poisoning would be
MEM_ROOT::ClearForReuse() which conceptually "frees" memory (so any access to
it should be flagged), but retains it in the MEM_ROOT, and may return it in
later allocations (and then it needs to be unpoisoned).  But the MEM_ROOT code
already follows the advice from the ASAN documentation mentioned earlier, so
ClearForReuse() is actually implemented with Clear() when running with ASAN. So
memory passed to ClearForReuse() is freed and suitably poisoned.

Thank you for considering my contribution. I agree that there is impedance mismatch between the MEM_ macro interface and ASan, which my patch tried to address by taking a quick small win with a minimal diff. If a single macro interface covering both ASan and Valgrind cannot be designed, then perhaps this whole abstraction should be removed and explicit Valgrind/ASan calls be made directly.