|
From: Nicholas N. <nj...@ca...> - 2004-07-10 14:56:39
|
CVS commit by nethercote:
Fix for bug #78048.
Problem was that the malloc-replacing tools (memcheck, addrcheck, massif,
helgrind) would assert if a too-big malloc was attempted. Now they return 0 to
the client. I also cleaned up the code handling heap-block-metadata in Massif
and Addrcheck/Memcheck a little.
This exposed a nasty bug in VG_(client_alloc)() which wasn't checking if
find_map_space() was succeeding before attempting an mmap(). Before I added
the check, very big mallocs (eg 2GB) for Addrcheck were overwriting the client
space at address 0 and causing crashes.
Added a regtest to all the affected skins for this.
A addrcheck/tests/toobig-allocs.stderr.exp 1.1
A addrcheck/tests/toobig-allocs.vgtest 1.1
A helgrind/tests/toobig-allocs.stderr.exp 1.1
A helgrind/tests/toobig-allocs.vgtest 1.1
A massif/tests/toobig-allocs.stderr.exp 1.1
A massif/tests/toobig-allocs.vgtest 1.1
A memcheck/tests/toobig-allocs.stderr.exp 1.1
A memcheck/tests/toobig-allocs.vgtest 1.1
A tests/toobig-allocs.c 1.1 [POSSIBLY UNSAFE: printf] [no copyright]
M +3 -1 addrcheck/tests/Makefile.am 1.9
M +14 -3 coregrind/vg_malloc2.c 1.25
M +5 -0 coregrind/vg_memory.c 1.53
M +3 -0 helgrind/hg_main.c 1.78
M +2 -1 helgrind/tests/Makefile.am 1.8
M +38 -44 massif/ms_main.c 1.12
M +1 -0 massif/tests/Makefile.am 1.2
M +35 -46 memcheck/mac_malloc_wrappers.c 1.13
M +2 -2 memcheck/mac_needs.c 1.29
M +3 -3 memcheck/mac_shared.h 1.20
M +1 -0 memcheck/tests/Makefile.am 1.37
M +1 -0 tests/.cvsignore 1.5
M +6 -4 tests/Makefile.am 1.36
--- valgrind/addrcheck/tests/Makefile.am #1.8:1.9
@@ -10,3 +10,5 @@
$(addsuffix .stdout.exp,$(INSN_TESTS)) \
$(addsuffix .vgtest,$(INSN_TESTS)) \
- overlap.stderr.exp overlap.stdout.exp overlap.vgtest
+ overlap.stderr.exp overlap.stdout.exp overlap.vgtest \
+ toobig-allocs.stderr.exp toobig-allocs.vgtest
+
--- valgrind/coregrind/vg_malloc2.c #1.24:1.25
@@ -327,10 +327,16 @@ Superblock* newSuperblock ( Arena* a, In
if (a->clientmem) {
+ // client allocation -- return 0 to client if it fails
sb = (Superblock *)
VG_(client_alloc)(0, cszW * sizeof(Word),
VKI_PROT_READ|VKI_PROT_WRITE|VKI_PROT_EXEC, 0);
- } else
+ if (NULL == sb) {
+ return 0;
+ }
+ } else {
+ // non-client allocation -- abort if it fails
sb = VG_(get_memory_from_mmap) ( cszW * sizeof(Word),
"newSuperblock" );
+ }
sb->n_payload_words = cszW - 2;
a->bytes_mmaped += cszW * sizeof(Word);
@@ -1024,5 +1030,10 @@ void* VG_(arena_malloc) ( ArenaId aid, I
req_bszW = pszW_to_bszW(a, req_pszW);
new_sb = newSuperblock(a, req_bszW);
- vg_assert(new_sb != NULL);
+ if (NULL == new_sb) {
+ // Should only fail if for client, otherwise, should have aborted
+ // already.
+ vg_assert(VG_AR_CLIENT == aid);
+ return NULL;
+ }
new_sb->next = a->sblocks;
a->sblocks = new_sb;
--- valgrind/coregrind/vg_memory.c #1.52:1.53
@@ -677,4 +677,5 @@ Bool VG_(is_addressable)(Addr p, Int siz
/*--------------------------------------------------------------------*/
+// Returns 0 on failure.
Addr VG_(client_alloc)(Addr addr, UInt len, UInt prot, UInt flags)
{
@@ -684,4 +685,8 @@ Addr VG_(client_alloc)(Addr addr, UInt l
addr = VG_(find_map_space)(addr, len, True);
+ // Don't do the mapping if we couldn't find space!
+ if (0 == addr)
+ return 0;
+
flags |= SF_CORE;
--- valgrind/helgrind/hg_main.c #1.77:1.78
@@ -1839,4 +1839,7 @@ void* alloc_and_new_mem ( Int size, UInt
p = (Addr)VG_(cli_malloc)(alignment, size);
+ if (!p) {
+ return NULL;
+ }
if (is_zeroed) VG_(memset)((void*)p, 0, size);
add_HG_Chunk ( VG_(get_current_or_recent_tid)(), p, size );
--- valgrind/helgrind/tests/Makefile.am #1.7:1.8
@@ -12,5 +12,6 @@
race.stderr.exp race.vgtest \
race2.stderr.exp race2.vgtest \
- readshared.stderr.exp readshared.vgtest
+ readshared.stderr.exp readshared.vgtest \
+ toobig-allocs.stderr.exp toobig-allocs.vgtest
check_PROGRAMS = \
--- valgrind/massif/ms_main.c #1.11:1.12
@@ -665,25 +665,36 @@ static void hp_census(void);
static __inline__
-void new_block_meta ( void* p, Int size, Bool custom_malloc )
+void* new_block ( void* p, Int size, UInt align, Bool is_zeroed )
{
HP_Chunk* hc;
+ Bool custom_alloc = (NULL == p);
+ if (size < 0) return NULL;
VGP_PUSHCC(VgpCliMalloc);
+ // Update statistics
+ n_allocs++;
if (0 == size) n_zero_allocs++;
+ // Allocate and zero if necessary
+ if (!p) {
+ p = VG_(cli_malloc)( align, size );
+ if (!p) {
+ VGP_POPCC(VgpCliMalloc);
+ return NULL;
+ }
+ if (is_zeroed) VG_(memset)(p, 0, size);
+ }
+
// Make new HP_Chunk node, add to malloclist
hc = VG_(malloc)(sizeof(HP_Chunk));
hc->size = size;
hc->data = (Addr)p;
-
+ hc->where = NULL; // paranoia
if (clo_heap) {
- hc->where = get_XCon( VG_(get_current_or_recent_tid)(), custom_malloc );
- if (size != 0)
+ hc->where = get_XCon( VG_(get_current_or_recent_tid)(), custom_alloc );
+ if (0 != size)
update_XCon(hc->where, size);
- } else {
- hc->where = NULL; // paranoia
}
-
add_HP_Chunk( hc );
@@ -688,24 +699,6 @@ void new_block_meta ( void* p, Int size,
add_HP_Chunk( hc );
- hp_census(); // do a census!
-
- VGP_POPCC(VgpCliMalloc);
-}
-
-static __inline__
-void* new_block ( Int size, UInt align, Bool is_zeroed )
-{
- void* p;
-
- if (size < 0) return NULL;
-
- VGP_PUSHCC(VgpCliMalloc);
-
- // Update statistics
- n_allocs++;
-
- p = VG_(cli_malloc)( align, size );
- if (is_zeroed) VG_(memset)(p, 0, size);
- new_block_meta(p, size, /*custom_malloc*/False);
+ // do a census!
+ hp_census();
VGP_POPCC(VgpCliMalloc);
@@ -716,6 +709,5 @@ static __inline__
void die_block ( void* p, Bool custom_free )
{
- HP_Chunk* hc;
- HP_Chunk** remove_handle;
+ HP_Chunk *hc, **remove_handle;
VGP_PUSHCC(VgpCliMalloc);
@@ -724,5 +716,6 @@ void die_block ( void* p, Bool custom_fr
n_frees++;
- hc = get_HP_Chunk ( p, &remove_handle );
+ // Remove HP_Chunk from malloclist
+ hc = get_HP_Chunk( p, &remove_handle );
if (hc == NULL)
return; // must have been a bogus free(), or p==NULL
@@ -727,20 +720,19 @@ void die_block ( void* p, Bool custom_fr
if (hc == NULL)
return; // must have been a bogus free(), or p==NULL
-
sk_assert(hc->data == (Addr)p);
+ remove_HP_Chunk(hc, remove_handle);
if (clo_heap && hc->size != 0)
update_XCon(hc->where, -hc->size);
- // Actually free the heap block
+ VG_(free)( hc );
+
+ // Actually free the heap block, if necessary
if (!custom_free)
VG_(cli_free)( p );
- // Remove HP_Chunk from malloclist, destroy
- remove_HP_Chunk(hc, remove_handle);
-
- hp_census(); // do a census!
+ // do a census!
+ hp_census();
- VG_(free)( hc );
VGP_POPCC(VgpCliMalloc);
}
@@ -749,25 +741,25 @@ void die_block ( void* p, Bool custom_fr
void* SK_(malloc) ( Int n )
{
- return new_block( n, VG_(clo_alignment), /*is_zeroed*/False );
+ return new_block( NULL, n, VG_(clo_alignment), /*is_zeroed*/False );
}
void* SK_(__builtin_new) ( Int n )
{
- return new_block( n, VG_(clo_alignment), /*is_zeroed*/False );
+ return new_block( NULL, n, VG_(clo_alignment), /*is_zeroed*/False );
}
void* SK_(__builtin_vec_new) ( Int n )
{
- return new_block( n, VG_(clo_alignment), /*is_zeroed*/False );
+ return new_block( NULL, n, VG_(clo_alignment), /*is_zeroed*/False );
}
void* SK_(calloc) ( Int m, Int size )
{
- return new_block( m*size, VG_(clo_alignment), /*is_zeroed*/True );
+ return new_block( NULL, m*size, VG_(clo_alignment), /*is_zeroed*/True );
}
void *SK_(memalign)( Int align, Int n )
{
- return new_block( n, align, False );
+ return new_block( NULL, n, align, False );
}
@@ -1134,8 +1126,10 @@ Bool SK_(handle_client_request) ( Thread
switch (argv[0]) {
case VG_USERREQ__MALLOCLIKE_BLOCK: {
+ void* res;
void* p = (void*)argv[1];
UInt sizeB = argv[2];
*ret = 0;
- new_block_meta( p, sizeB, /*custom_malloc*/True );
+ res = new_block( p, sizeB, /*align -- ignored*/0, /*is_zeroed*/False );
+ sk_assert(res == p);
return True;
}
--- valgrind/massif/tests/Makefile.am #1.1:1.2
@@ -2,4 +2,5 @@
EXTRA_DIST = $(noinst_SCRIPTS) \
+ toobig-allocs.stderr.exp toobig-allocs.vgtest \
true_html.stderr.exp true_html.vgtest \
true_text.stderr.exp true_text.vgtest
--- valgrind/memcheck/mac_malloc_wrappers.c #1.12:1.13
@@ -133,6 +133,5 @@ MAC_Chunk* MAC_(first_matching_freed_MAC
/* Allocate its shadow chunk, put it on the appropriate list. */
static
-MAC_Chunk* add_MAC_Chunk ( Addr p, UInt size, MAC_AllocKind kind,
- VgHashTable table)
+void add_MAC_Chunk ( Addr p, UInt size, MAC_AllocKind kind, VgHashTable table)
{
MAC_Chunk* mc;
@@ -154,6 +153,4 @@ MAC_Chunk* add_MAC_Chunk ( Addr p, UInt
VG_(HT_add_node)( table, (VgHashNode*)mc );
-
- return mc;
}
@@ -164,12 +161,8 @@ MAC_Chunk* add_MAC_Chunk ( Addr p, UInt
/* Allocate memory and note change in memory available */
__inline__
-MAC_Chunk* MAC_(new_block) ( Addr p, UInt size,
- UInt rzB, Bool is_zeroed, MAC_AllocKind kind,
- VgHashTable table)
+void* MAC_(new_block) ( Addr p, UInt size, UInt align, UInt rzB,
+ Bool is_zeroed, MAC_AllocKind kind, VgHashTable table)
{
- MAC_Chunk *mc;
-
VGP_PUSHCC(VgpCliMalloc);
-
cmalloc_n_mallocs ++;
cmalloc_bs_mallocd += size;
@@ -175,5 +168,18 @@ MAC_Chunk* MAC_(new_block) ( Addr p, UIn
cmalloc_bs_mallocd += size;
- mc = add_MAC_Chunk( p, size, kind, table );
+ // Allocate and zero if necessary
+ if (p) {
+ sk_assert(MAC_AllocCustom == kind);
+ } else {
+ sk_assert(MAC_AllocCustom != kind);
+ p = (Addr)VG_(cli_malloc)( align, size );
+ if (!p) {
+ VGP_POPCC(VgpCliMalloc);
+ return NULL;
+ }
+ if (is_zeroed) VG_(memset)((void*)p, 0, size);
+ }
+
+ add_MAC_Chunk( p, size, kind, table );
MAC_(ban_mem_heap)( p-rzB, rzB );
@@ -183,5 +189,5 @@ MAC_Chunk* MAC_(new_block) ( Addr p, UIn
VGP_POPCC(VgpCliMalloc);
- return mc;
+ return (void*)p;
}
@@ -192,9 +198,7 @@ void* SK_(malloc) ( Int n )
return NULL;
} else {
- Addr p = (Addr)VG_(cli_malloc)( VG_(clo_alignment), n );
- MAC_(new_block) ( p, n, VG_(vg_malloc_redzone_szB),
- /*is_zeroed*/False, MAC_AllocMalloc,
+ return MAC_(new_block) ( 0, n, VG_(clo_alignment),
+ VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocMalloc,
MAC_(malloc_list));
- return (void*)p;
}
}
@@ -206,9 +210,7 @@ void* SK_(__builtin_new) ( Int n )
return NULL;
} else {
- Addr p = (Addr)VG_(cli_malloc)( VG_(clo_alignment), n );
- MAC_(new_block) ( p, n, VG_(vg_malloc_redzone_szB),
- /*is_zeroed*/False, MAC_AllocNew,
+ return MAC_(new_block) ( 0, n, VG_(clo_alignment),
+ VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocNew,
MAC_(malloc_list));
- return (void*)p;
}
}
@@ -221,9 +223,7 @@ void* SK_(__builtin_vec_new) ( Int n )
return NULL;
} else {
- Addr p = (Addr)VG_(cli_malloc)( VG_(clo_alignment), n );
- MAC_(new_block) ( p, n, VG_(vg_malloc_redzone_szB),
- /*is_zeroed*/False, MAC_AllocNewVec,
+ return MAC_(new_block) ( 0, n, VG_(clo_alignment),
+ VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocNewVec,
MAC_(malloc_list));
- return (void*)p;
}
}
@@ -235,9 +235,7 @@ void* SK_(memalign) ( Int align, Int n )
return NULL;
} else {
- Addr p = (Addr)VG_(cli_malloc)( align, n );
- MAC_(new_block) ( p, n, VG_(vg_malloc_redzone_szB),
- /*is_zeroed*/False, MAC_AllocMalloc,
+ return MAC_(new_block) ( 0, n, align,
+ VG_(vg_malloc_redzone_szB), /*is_zeroed*/False, MAC_AllocMalloc,
MAC_(malloc_list));
- return (void*)p;
}
}
@@ -245,8 +243,4 @@ void* SK_(memalign) ( Int align, Int n )
void* SK_(calloc) ( Int nmemb, Int size1 )
{
- Int n, i;
-
- n = nmemb * size1;
-
if (nmemb < 0 || size1 < 0) {
VG_(message)(Vg_UserMsg, "Warning: silly args (%d,%d) to calloc()",
@@ -254,11 +248,7 @@ void* SK_(calloc) ( Int nmemb, Int size1
return NULL;
} else {
- Addr p = (Addr)VG_(cli_malloc)( VG_(clo_alignment), n );
- MAC_(new_block) ( p, n, VG_(vg_malloc_redzone_szB),
- /*is_zeroed*/True, MAC_AllocMalloc,
+ return MAC_(new_block) ( 0, nmemb*size1, VG_(clo_alignment),
+ VG_(vg_malloc_redzone_szB), /*is_zeroed*/True, MAC_AllocMalloc,
MAC_(malloc_list));
- for (i = 0; i < n; i++)
- ((UChar*)p)[i] = 0;
- return (void*)p;
}
}
@@ -480,5 +470,4 @@ void MAC_(mempool_alloc)(Addr pool, Addr
MAC_Mempool* mp;
MAC_Mempool** prev_next;
- MAC_Chunk* mc;
mp = (MAC_Mempool*)VG_(HT_get_node) ( MAC_(mempool_list), (UInt)pool,
@@ -492,6 +481,6 @@ void MAC_(mempool_alloc)(Addr pool, Addr
}
- mc = MAC_(new_block)(addr, size, mp->rzB, mp->is_zeroed, MAC_AllocCustom,
- mp->chunks);
+ MAC_(new_block)(addr, size, /*ignored*/0, mp->rzB, mp->is_zeroed,
+ MAC_AllocCustom, mp->chunks);
}
--- valgrind/memcheck/mac_needs.c #1.28:1.29
@@ -881,6 +881,6 @@ Bool MAC_(handle_common_client_requests)
Bool is_zeroed = (Bool)arg[4];
- MAC_(new_block) ( p, sizeB, rzB, is_zeroed, MAC_AllocCustom,
- MAC_(malloc_list) );
+ MAC_(new_block) ( p, sizeB, /*ignored*/0, rzB, is_zeroed,
+ MAC_AllocCustom, MAC_(malloc_list) );
return True;
}
--- valgrind/memcheck/mac_shared.h #1.19:1.20
@@ -318,5 +318,5 @@ extern void MAC_(clear_MAC_Error)
extern Bool MAC_(shared_recognised_suppression) ( Char* name, Supp* su );
-extern MAC_Chunk* MAC_(new_block) ( Addr p, UInt size, UInt rzB,
+extern void* MAC_(new_block) ( Addr p, UInt size, UInt align, UInt rzB,
Bool is_zeroed, MAC_AllocKind kind,
VgHashTable table);
--- valgrind/memcheck/tests/Makefile.am #1.36:1.37
@@ -65,4 +65,5 @@
supp.supp \
suppfree.stderr.exp suppfree.vgtest \
+ toobig-allocs.stderr.exp toobig-allocs.vgtest \
trivialleak.stderr.exp trivialleak.vgtest \
tronical.stderr.exp tronical.vgtest \
--- valgrind/tests/.cvsignore #1.4:1.5
@@ -3,3 +3,4 @@
cputest
vg_regtest
+toobig-allocs
true
--- valgrind/tests/Makefile.am #1.35:1.36
@@ -13,6 +13,7 @@
check_PROGRAMS = \
- true \
- cputest
+ cputest \
+ toobig-allocs \
+ true
AM_CFLAGS = $(WERROR) -Winline -Wall -Wshadow -g
@@ -20,5 +21,6 @@
# generic C ones
-true_SOURCES = true.c
-cputest_SOURCES = cputest.c
+cputest_SOURCES = cputest.c
+toobig_allocs_SOURCES = toobig-allocs.c
+true_SOURCES = true.c
|