Summary: | Add mismatched detection to C++ 17 aligned new/delete | ||
---|---|---|---|
Product: | [Developer tools] valgrind | Reporter: | Paul Floyd <pjfloyd> |
Component: | memcheck | Assignee: | Paul Floyd <pjfloyd> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | sam |
Priority: | NOR | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Platform: | Other | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Attachments: |
Big patch for lots of aligned / sized alloc errors
Big patch for lots of aligned / sized alloc errors Big patch for lots of aligned / sized alloc errors Big patch for lots of aligned / sized alloc errors Big patch for lots of aligned / sized alloc errors Big patch for lots of aligned / sized alloc errors |
Description
Paul Floyd
2021-03-02 13:10:08 UTC
I also recently noticed that C23 will probably add new versions of free (free_sized and free_aligned_sized but no free_aligned). As I see it, these are both optional and a conforming implementation can just call free. But it's UB to use the wrong size or alignment. Mixing aligned_alloc/free_sized and plain malloc family/free_aligned_sized is not allowed. From a memcheck perspective there are a few issues. The main one that I see is where to store all this info. We currently use a 2 bit bitfield in MC_Chunk to encode malloc/new/new[]/custom. This isn't too bad for 64bit platforms, but it does limit the block size on 32bit. Now we need malloc, new and new[] which are mutually exclusive. I don't think that we need to encode sized since pretty much by definition all alocations are sized. That means we need 2 more bits to encode custom and aligned (there's a bugzi somewhere that asks for custom to also work for new/new[]). I haven't looks but I imagine that we also need to store the alignment somewhere. To be safe that means a whole register word field. So on 32bit that means adding 2 (or 1 with some squeezing) RegWords to MC_Chunk. For the C++ aligned allocators libc++ bumps up the allocation size if it is less than sizeof(void*) and then uses posix_memalign libstdc++ does a popcnt (!!!) to check for power of 2 then calls aligned_alloc (actually it looks like a feature check for aligned_alloc _aligned_malloc (Windows) posix_memalign and then memalign), It will also bump up size to be an integer multiple of I need to look at the standard to see what size triggers use of aligned allocators. For libstdc++ look in ibstdc++-v3/libsupc++/new_opa.cc foo.cpp:5:19: error: requested alignment '67' is not a positive power of 2 5 | class alignas(67) MyClass { | ^~~~~~~ A bit of a fight to get the compiler to do the wrong thing. MyClass* myClass = (MyClass *)operator new(sizeof(MyClass), std::align_val_t(67U)); with libstdc++ > 122 if (__builtin_expect (!std::__has_single_bit(align), false)) │ │ 123 _GLIBCXX_THROW_OR_ABORT(bad_alloc()); gives terminate called after throwing an instance of 'std::bad_alloc' what(): std::bad_alloc Program received signal SIGABRT, Aborted. The wrappers used for new / delete operators now takes the size and alignment and will generate warnings if they are uninit. The nothrow tag gets ignored - it's only there for overload resolution. Created attachment 157739 [details] Big patch for lots of aligned / sized alloc errors This also covers https://bugs.kde.org/show_bug.cgi?id=433859 https://bugs.kde.org/show_bug.cgi?id=467441 https://bugs.kde.org/show_bug.cgi?id=466105 So far I've done basic testing on FreeBSD amd64, FreeBSD x86, Linux glibx amd64, Linux musl amd64 and Illumos. I'll have a go with older Solaris and macOS at a later date. A few todos: Updated the documentation. Figure out how to add the new user rteq to memcheck.h and include that in vg_replace_malloc.c There are a couple of design decisions that I might rework. 1. Lots of stuff is being done in the vg_replace_malloc.c macro wrappers. Specifically there's a new client request in addition to all of the existing checks and alignment fixup. This could probably be done differently, moving all the code to some common functions in replacemalloc_core.c and then doing the memcheck stuff in the respective wrappers. 2. I've added a tag to the replacement alloc fn macros so that I can have enum names like AllocKindVecDeleteSizedAligned . I could reuse the string used for the function name to call, but that would give me enum names like alloc_kind___builtin_vec_delete_aligned_sized which I find ugly. Created attachment 157920 [details]
Big patch for lots of aligned / sized alloc errors
A few updates to expecteds, alignment of 0 not allowed for memalign on Illumos
Created attachment 157921 [details]
Big patch for lots of aligned / sized alloc errors
Fixes for macOS
Created attachment 157936 [details]
Big patch for lots of aligned / sized alloc errors
Last macOS and doc changes
Created attachment 158590 [details]
Big patch for lots of aligned / sized alloc errors
Use memcheck.h for internal use user req. Had problems including it previously, not sure what I did wrong.
I need to make sure that I didn't make the same error here as I did with realloc size zero (I suspect so). Will double check and add some tests. Created attachment 159643 [details]
Big patch for lots of aligned / sized alloc errors
Fix missing cases for MC_(eq_Error) and a testcase that generates duplicate errors of each kind and same alignment/size.
Also fixed. |