|
From: Bart V. A. <bva...@ac...> - 2011-02-28 18:05:17
|
The existing custom memory allocator support in Valgrind works great for custom allocators that only allocate and free memory and that do not support in-place reallocation. Implementing a custom allocator that supports in-place reallocation and at the same time preserves the V-bit information managed by memcheck is currently not possible without performing an additional allocation for temporarily storing the V-bit information. Hence the proposal to add a client request VG_USERREQ__REALLOCLIKE_BLOCK and a macro VALGRIND_REALLOCLIKE_BLOCK() as is done in the patch below. There is at least one custom allocator that will benefit of this and that is the heap allocator in Wine. A patch for Wine that uses this new client request can be found here: https://github.com/bvanassche/wine/commit/03b56e8b217a7cf5663fdf23a39dcd686babde02. Note: both patches have only been tested lightly so far. Yet I'm posting these patches on the Valgrind-developers mailing list to invite feedback about the chosen approach. Index: docs/xml/manual-core-adv.xml =================================================================== --- docs/xml/manual-core-adv.xml (revision 11577) +++ docs/xml/manual-core-adv.xml (working copy) @@ -152,6 +152,15 @@ tool-specific macros).</para> </varlistentry> <varlistentry> + <term><command><computeroutput>VALGRIND_REALLOCLIKE_BLOCK</computeroutput>:</command></term> + <listitem> + <para>Informs a Valgrind tool that the size of an allocated block has been + modified but not its address. See <filename>valgrind.h</filename> for + more information on how to use it.</para> + </listitem> + </varlistentry> + + <varlistentry> <term> <command><computeroutput>VALGRIND_CREATE_MEMPOOL</computeroutput></command>, <command><computeroutput>VALGRIND_DESTROY_MEMPOOL</computeroutput></command>, Index: memcheck/tests/custom_alloc.c =================================================================== --- memcheck/tests/custom_alloc.c (revision 11577) +++ memcheck/tests/custom_alloc.c (working copy) @@ -53,7 +53,7 @@ static void custom_free(void* p) // don't actually free any memory... but mark it as freed VALGRIND_FREELIKE_BLOCK( p, RZ ); } -#undef RZ + @@ -78,6 +78,16 @@ int main(void) array[9] = 8; array[10] = 10; // invalid write (ok w/o MALLOCLIKE -- in superblock) + VALGRIND_REALLOCLIKE_BLOCK(array, sizeof(int) * 10, sizeof(int) * 5, RZ); + array[4] = 7; + array[5] = 9; // invalid write + + VALGRIND_REALLOCLIKE_BLOCK(array, sizeof(int) * 5, sizeof(int) * 10, RZ); + if (array[5]) array[4]++; // uninitialized read of array[5] + array[5] = 11; + array[9] = 7; + array[10] = 8; // invalid write + custom_free(array); // ok custom_free((void*)0x1); // invalid free Index: memcheck/tests/custom_alloc.stderr.exp =================================================================== --- memcheck/tests/custom_alloc.stderr.exp (revision 11577) +++ memcheck/tests/custom_alloc.stderr.exp (working copy) @@ -4,21 +4,36 @@ Invalid write of size 4 at 0x........: custom_alloc (custom_alloc.c:47) by 0x........: main (custom_alloc.c:76) +Invalid write of size 4 + at 0x........: main (custom_alloc.c:83) + Address 0x........ is 0 bytes after a block of size 20 alloc'd + at 0x........: custom_alloc (custom_alloc.c:47) + by 0x........: main (custom_alloc.c:76) + +Conditional jump or move depends on uninitialised value(s) + at 0x........: main (custom_alloc.c:86) + +Invalid write of size 4 + at 0x........: main (custom_alloc.c:89) + Address 0x........ is 0 bytes after a block of size 40 alloc'd + at 0x........: custom_alloc (custom_alloc.c:47) + by 0x........: main (custom_alloc.c:76) + Invalid free() / delete / delete[] at 0x........: custom_free (custom_alloc.c:54) - by 0x........: main (custom_alloc.c:83) + by 0x........: main (custom_alloc.c:93) Address 0x........ is not stack'd, malloc'd or (recently) free'd Mismatched free() / delete / delete [] at 0x........: custom_free (custom_alloc.c:54) - by 0x........: main (custom_alloc.c:86) + by 0x........: main (custom_alloc.c:96) Address 0x........ is 0 bytes inside a block of size 40 alloc'd at 0x........: malloc (vg_replace_malloc.c:...) - by 0x........: main (custom_alloc.c:85) + by 0x........: main (custom_alloc.c:95) Invalid read of size 4 - at 0x........: main (custom_alloc.c:89) + at 0x........: main (custom_alloc.c:99) Address 0x........ is 0 bytes inside a block of size 40 free'd at 0x........: custom_free (custom_alloc.c:54) - by 0x........: main (custom_alloc.c:81) + by 0x........: main (custom_alloc.c:91) Index: memcheck/mc_main.c =================================================================== --- memcheck/mc_main.c (revision 11577) +++ memcheck/mc_main.c (working copy) @@ -4986,16 +4986,17 @@ static Bool mc_handle_client_request ( T Addr bad_addr; if (!VG_IS_TOOL_USERREQ('M','C',arg[0]) - && VG_USERREQ__MALLOCLIKE_BLOCK != arg[0] - && VG_USERREQ__FREELIKE_BLOCK != arg[0] - && VG_USERREQ__CREATE_MEMPOOL != arg[0] - && VG_USERREQ__DESTROY_MEMPOOL != arg[0] - && VG_USERREQ__MEMPOOL_ALLOC != arg[0] - && VG_USERREQ__MEMPOOL_FREE != arg[0] - && VG_USERREQ__MEMPOOL_TRIM != arg[0] - && VG_USERREQ__MOVE_MEMPOOL != arg[0] - && VG_USERREQ__MEMPOOL_CHANGE != arg[0] - && VG_USERREQ__MEMPOOL_EXISTS != arg[0]) + && VG_USERREQ__MALLOCLIKE_BLOCK != arg[0] + && VG_USERREQ__REALLOCLIKE_BLOCK != arg[0] + && VG_USERREQ__FREELIKE_BLOCK != arg[0] + && VG_USERREQ__CREATE_MEMPOOL != arg[0] + && VG_USERREQ__DESTROY_MEMPOOL != arg[0] + && VG_USERREQ__MEMPOOL_ALLOC != arg[0] + && VG_USERREQ__MEMPOOL_FREE != arg[0] + && VG_USERREQ__MEMPOOL_TRIM != arg[0] + && VG_USERREQ__MOVE_MEMPOOL != arg[0] + && VG_USERREQ__MEMPOOL_CHANGE != arg[0] + && VG_USERREQ__MEMPOOL_EXISTS != arg[0]) return False; switch (arg[0]) { @@ -5121,6 +5122,15 @@ static Bool mc_handle_client_request ( T MC_AllocCustom, MC_(malloc_list) ); return True; } + case VG_USERREQ__REALLOCLIKE_BLOCK: { + Addr p = (Addr)arg[1]; + SizeT oldSizeB = arg[2]; + SizeT newSizeB = arg[3]; + //UInt rzB = arg[4]; XXX: unused! + + MC_(handle_realloc) ( tid, p, oldSizeB, newSizeB ); + return True; + } case VG_USERREQ__FREELIKE_BLOCK: { Addr p = (Addr)arg[1]; UInt rzB = arg[2]; Index: memcheck/mc_include.h =================================================================== --- memcheck/mc_include.h (revision 11577) +++ memcheck/mc_include.h (working copy) @@ -83,6 +83,8 @@ void* MC_(new_block) ( ThreadId tid, Addr p, SizeT size, SizeT align, Bool is_zeroed, MC_AllocKind kind, VgHashTable table); +void MC_(handle_realloc) ( ThreadId tid, + Addr p, SizeT oldSizeB, SizeT newSizeB ); void MC_(handle_free) ( ThreadId tid, Addr p, UInt rzB, MC_AllocKind kind ); Index: memcheck/mc_malloc_wrappers.c =================================================================== --- memcheck/mc_malloc_wrappers.c (revision 11577) +++ memcheck/mc_malloc_wrappers.c (working copy) @@ -280,6 +280,34 @@ void* MC_(calloc) ( ThreadId tid, SizeT } } +void MC_(handle_realloc)(ThreadId tid, Addr p, SizeT oldSizeB, SizeT newSizeB) +{ + MC_Chunk* mc; + + if (oldSizeB == newSizeB) + return; + + mc = VG_(HT_lookup) ( MC_(malloc_list), (UWord)p ); + tl_assert(mc); + tl_assert(mc->szB == oldSizeB); + tl_assert(newSizeB); + mc->szB = newSizeB; + if (newSizeB < oldSizeB) + { + MC_(make_mem_noaccess)( p + newSizeB, oldSizeB - newSizeB ); + } + else + { + ExeContext* ec; + UInt ecu; + + ec = VG_(record_ExeContext)(tid, 0/*first_ip_delta*/); + ecu = VG_(get_ECU_from_ExeContext)(ec); + MC_(make_mem_undefined_w_otag)( p + oldSizeB, newSizeB - oldSizeB, + ecu | MC_OKIND_HEAP ); + } +} + static void die_and_free_mem ( ThreadId tid, MC_Chunk* mc, SizeT rzB ) { Index: massif/ms_main.c =================================================================== --- massif/ms_main.c (revision 11577) +++ massif/ms_main.c (working copy) @@ -1988,6 +1988,15 @@ static Bool ms_handle_client_request ( T *ret = 0; return True; } + case VG_USERREQ__REALLOCLIKE_BLOCK: { + void* p = (void*)argv[1]; + SizeT newSizeB = argv[3]; + + unrecord_block(p, /*maybe_snapshot*/True); + record_block(tid, p, newSizeB, /*slop_szB*/0, + /*exclude_first_entry*/False, /*maybe_snapshot*/True); + return True; + } case VG_USERREQ__FREELIKE_BLOCK: { void* p = (void*)argv[1]; unrecord_block(p, /*maybe_snapshot*/True); Index: include/valgrind.h =================================================================== --- include/valgrind.h (revision 11577) +++ include/valgrind.h (working copy) @@ -4295,6 +4295,7 @@ typedef /* These are useful and can be interpreted by any tool that tracks malloc() et al, by using vg_replace_malloc.c. */ VG_USERREQ__MALLOCLIKE_BLOCK = 0x1301, + VG_USERREQ__REALLOCLIKE_BLOCK= 0x130b, VG_USERREQ__FREELIKE_BLOCK = 0x1302, /* Memory pool support. */ VG_USERREQ__CREATE_MEMPOOL = 0x1303, @@ -4628,7 +4629,21 @@ VALGRIND_PRINTF_BACKTRACE(const char *fo VALGRIND_FREELIKE_BLOCK should be put immediately after the point where a heap block is deallocated. - In many cases, these two client requests will not be enough to get your + VALGRIND_REALLOCLIKE_BLOCK informs a tool about reallocation. For + Memcheck, it does three things: + + - It records that the size of a block has been changed. This assumes that + the block was annotated as having been allocated via + VALGRIND_MALLOCLIKE_BLOCK. Otherwise, an error will be issued. + + - If the block shrunk, it marks the freed memory as being unaddressable. + + - The V-bits of the overlap between the old and the new block are preserved. + + VALGRIND_REALLOCLIKE_BLOCK should be put after allocation of the new block + and before deallocation of the old block. + + In many cases, these three client requests will not be enough to get your allocator working well with Memcheck. More specifically, if your allocator writes to freed blocks in any way then a VALGRIND_MAKE_MEM_UNDEFINED call will be necessary to mark the memory as addressable just before the zeroing @@ -4646,9 +4661,6 @@ VALGRIND_PRINTF_BACKTRACE(const char *fo understand the distinction between the allocator and the rest of the program. - Note: there is currently no VALGRIND_REALLOCLIKE_BLOCK client request; it - has to be emulated with MALLOCLIKE/FREELIKE and memory copying. - Ignored if addr == 0. */ #define VALGRIND_MALLOCLIKE_BLOCK(addr, sizeB, rzB, is_zeroed) \ @@ -4659,6 +4671,16 @@ VALGRIND_PRINTF_BACKTRACE(const char *fo } /* See the comment for VALGRIND_MALLOCLIKE_BLOCK for details. + Ignored if addr == 0. +*/ +#define VALGRIND_REALLOCLIKE_BLOCK(addr, oldSizeB, newSizeB, rzB)\ + {unsigned int _qzz_res; \ + VALGRIND_DO_CLIENT_REQUEST(_qzz_res, 0, \ + VG_USERREQ__REALLOCLIKE_BLOCK, \ + addr, oldSizeB, newSizeB, rzB, 0); \ + } + +/* See the comment for VALGRIND_MALLOCLIKE_BLOCK for details. Ignored if addr == 0. */ #define VALGRIND_FREELIKE_BLOCK(addr, rzB) \ Index: drd/drd_clientreq.c =================================================================== --- drd/drd_clientreq.c (revision 11577) +++ drd/drd_clientreq.c (working copy) @@ -80,6 +80,22 @@ static Bool handle_client_request(Thread DRD_(malloclike_block)(vg_tid, arg[1]/*addr*/, arg[2]/*size*/); break; + case VG_USERREQ__REALLOCLIKE_BLOCK: + if (!DRD_(freelike_block)(vg_tid, arg[1]/*addr*/)) + { + GenericErrInfo GEI = { + .tid = DRD_(thread_get_running_tid)(), + .addr = 0, + }; + VG_(maybe_record_error)(vg_tid, + GenericErr, + VG_(get_IP)(vg_tid), + "Invalid VG_USERREQ__REALLOCLIKE_BLOCK request", + &GEI); + } + DRD_(malloclike_block)(vg_tid, arg[1]/*addr*/, arg[3]/*newSize*/); + break; + case VG_USERREQ__FREELIKE_BLOCK: if (arg[1] && ! DRD_(freelike_block)(vg_tid, arg[1]/*addr*/)) { Index: coregrind/m_scheduler/scheduler.c =================================================================== --- coregrind/m_scheduler/scheduler.c (revision 11577) +++ coregrind/m_scheduler/scheduler.c (working copy) @@ -1577,6 +1577,7 @@ void do_client_request ( ThreadId tid ) } case VG_USERREQ__MALLOCLIKE_BLOCK: + case VG_USERREQ__REALLOCLIKE_BLOCK: case VG_USERREQ__FREELIKE_BLOCK: // Ignore them if the addr is NULL; otherwise pass onto the tool. if (!arg[1]) { |