|
From: <sv...@va...> - 2012-02-26 21:30:34
|
Author: philippe
Date: 2012-02-26 21:26:00 +0000 (Sun, 26 Feb 2012)
New Revision: 12405
Log:
Fix some memory leaks found by running memcheck on annotated memcheck.
Modified:
trunk/coregrind/m_errormgr.c
trunk/memcheck/mc_leakcheck.c
Modified: trunk/coregrind/m_errormgr.c
===================================================================
--- trunk/coregrind/m_errormgr.c 2012-02-26 17:51:28 UTC (rev 12404)
+++ trunk/coregrind/m_errormgr.c 2012-02-26 21:26:00 UTC (rev 12405)
@@ -1103,26 +1103,30 @@
}
-/* *p_caller contains the raw name of a caller, supposedly either
+/* buf contains the raw name of a caller, supposedly either
fun:some_function_name or
- obj:some_object_name.
- Set *p_ty accordingly and advance *p_caller over the descriptor
- (fun: or obj:) part.
+ obj:some_object_name or
+ ...
+ Set p->ty and p->name accordingly.
+ p->name is allocated and set to the string
+ after the descriptor (fun: or obj:) part.
Returns False if failed.
*/
-static Bool setLocationTy ( SuppLoc* p )
+static Bool setLocationTy ( SuppLoc* p, Char *buf )
{
- if (VG_(strncmp)(p->name, "fun:", 4) == 0) {
- p->name += 4;
+ if (VG_(strncmp)(buf, "fun:", 4) == 0) {
+ p->name = VG_(arena_strdup)(VG_AR_CORE,
+ "errormgr.sLTy.1", buf+4);
p->ty = FunName;
return True;
}
- if (VG_(strncmp)(p->name, "obj:", 4) == 0) {
- p->name += 4;
+ if (VG_(strncmp)(buf, "obj:", 4) == 0) {
+ p->name = VG_(arena_strdup)(VG_AR_CORE,
+ "errormgr.sLTy.2", buf+4);
p->ty = ObjName;
return True;
}
- if (VG_(strcmp)(p->name, "...") == 0) {
+ if (VG_(strcmp)(buf, "...") == 0) {
p->name = NULL;
p->ty = DotDotDot;
return True;
@@ -1200,7 +1204,10 @@
supp->string = supp->extra = NULL;
eof = VG_(get_line) ( fd, &buf, &nBuf, &lineno );
- if (eof) break;
+ if (eof) {
+ VG_(arena_free)(VG_AR_CORE, supp);
+ break;
+ }
if (!VG_STREQ(buf, "{")) BOMB("expected '{' or end-of-file");
@@ -1253,6 +1260,8 @@
if (VG_STREQ(buf, "}"))
break;
}
+ VG_(arena_free)(VG_AR_CORE, supp->sname);
+ VG_(arena_free)(VG_AR_CORE, supp);
continue;
}
@@ -1280,9 +1289,7 @@
BOMB("too many callers in stack trace");
if (i > 0 && i >= VG_(clo_backtrace_size))
break;
- tmp_callers[i].name = VG_(arena_strdup)(VG_AR_CORE,
- "errormgr.losf.3", buf);
- if (!setLocationTy(&(tmp_callers[i])))
+ if (!setLocationTy(&(tmp_callers[i]), buf))
BOMB("location should be \"...\", or should start "
"with \"fun:\" or \"obj:\"");
i++;
Modified: trunk/memcheck/mc_leakcheck.c
===================================================================
--- trunk/memcheck/mc_leakcheck.c 2012-02-26 17:51:28 UTC (rev 12404)
+++ trunk/memcheck/mc_leakcheck.c 2012-02-26 21:26:00 UTC (rev 12405)
@@ -1276,6 +1276,7 @@
/*clique*/-1, /*cur_clique*/-1,
searched, szB);
}
+ VG_(free)(seg_starts);
}
/*------------------------------------------------------------*/
|
|
From: Philippe W. <phi...@sk...> - 2012-02-26 21:58:43
|
On Sun, 2012-02-26 at 21:26 +0000, sv...@va... wrote: > Author: philippe > Date: 2012-02-26 21:26:00 +0000 (Sun, 26 Feb 2012) > New Revision: 12405 > > Log: > Fix some memory leaks found by running memcheck on annotated memcheck. Note that there is one reported leak I did not fix, as it is not easy to fix, and it is a one shot leak. ==11755== 392 bytes in 1 blocks are definitely lost in loss record 66 of 115 ==11755== at 0x2803A155: vgPlain_arena_malloc (m_mallocfree.c:1599) ==11755== by 0x2803A7CF: vgPlain_malloc (m_mallocfree.c:2125) ==11755== by 0x28073610: vgPlain_ii_create_image (initimg-linux.c:202) ==11755== by 0x280326DF: valgrind_main (m_main.c:1718) ==11755== by 0x28035E3C: _start_in_C_linux (m_main.c:2797) ==11755== by 0x28030A5B: ??? (in /home/philippe/valgrind/memcheck_inner_annot/install/lib/valgrind/memcheck-x86-linux) |
|
From: Julian S. <js...@ac...> - 2012-03-02 10:56:46
|
> Note that there is one reported leak I did not fix, as it is not easy
> to fix, and it is a one shot leak.
>
> ==11755== 392 bytes in 1 blocks are definitely lost in loss record 66 of
> 115 ==11755== at 0x2803A155: vgPlain_arena_malloc (m_mallocfree.c:1599)
> ==11755== by 0x2803A7CF: vgPlain_malloc (m_mallocfree.c:2125)
> ==11755== by 0x28073610: vgPlain_ii_create_image (initimg-linux.c:202)
> ==11755== by 0x280326DF: valgrind_main (m_main.c:1718)
> ==11755== by 0x28035E3C: _start_in_C_linux (m_main.c:2797)
> ==11755== by 0x28030A5B: ??? (in
> /home/philippe/valgrind/memcheck_inner_annot/install/lib/valgrind/memcheck
> -x86-linux)
Yes, looks harmless.
Thinking about this X-on-memcheck game a bit more .. it's great that
you found some leaks. I am a bit surprised though that you didn't
find any uninitialised value or out-of-range errors w.r.t. heap
blocks though (300+ kloc, never Memcheck'd before, no heap errors?!)
and this makes me wonder if the inner annotations for heap block
allocation are working correctly.
One way you can test this is to introduce some error into m_main.c,
near here ..
VG_(debugLog)(1, "main", "Starting the dynamic memory manager\n");
{ void* p = VG_(malloc)( "main.vm.1", 12345 );
if (p) VG_(free)( p );
}
for example (having allocated p)
if ( ((UInt*)p)[123] == 456) VG_(printf)("foo"); else VG_(printf)("bar");
if ( ((UInt*)p)[-1] == 456) VG_(printf)("sheesh kebab");
char local[123];
if (local[27] + local[81] == 12) printf("baah");
kind of thing .. it would be nice to know that we see an error from
all 3.
J
|
|
From: Philippe W. <phi...@sk...> - 2012-03-04 23:10:36
|
On Fri, 2012-03-02 at 11:54 +0100, Julian Seward wrote:
> Thinking about this X-on-memcheck game a bit more .. it's great that
> you found some leaks. I am a bit surprised though that you didn't
> find any uninitialised value or out-of-range errors w.r.t. heap
> blocks though (300+ kloc, never Memcheck'd before, no heap errors?!)
> and this makes me wonder if the inner annotations for heap block
> allocation are working correctly.
...
> kind of thing .. it would be nice to know that we see an error from
> all 3.
VG_(debugLog)(1, "main", "Starting the dynamic memory manager\n");
{ void* p = VG_(malloc)( "main.vm.1", 12345 );
char local[123];
if (argc > 100) {local[27] = argc; local[81] = VG_(strlen)(argv[1]);}
if (local[27] + local[81] >= 12) VG_(printf)("baah %d", local[27]); //<<<< Error detected
if ( ((UInt*)p)[123] == 456) VG_(printf)("foo"); else VG_(printf)("bar"); //<<<< Error detected
if ( ((UInt*)p)[-1] == 456) VG_(printf)("sheesh kebab");
if (p) VG_(free)( p );
if ( ((UInt*)p)[345] == 456) VG_(printf)("used after free"); else VG_(printf)("after free bar"); //<<<<< Error detected
}
VG_(debugLog)(1, "main", "Dynamic memory manager is running\n");
I had to persuade the compiler to not remove the local[123] before
I got an error for this one.
For what concerns the p[-1] : I think there is no error detected
because the memcheck annotations need at least two improvements:
* they do not "declare" the red zone to the outer valgrind.
* when a block is freed, the memory is marked undefined, while
it should be marked no access.
But m_mallocfree.c needs to access the free blocks (e.g.
to give it back as the next block).
So, there is a need to mark the memory "no access" when
freed, then temporarily accessible again when m_mallocfree.c
needs it for its internal business, then marking it again
no access (till it is really allocated).
The first thing is probably easy.
The second looks significantly more tricky (and might make the
outer/inner combination even slower as any m_mallocfree.c
free or alloc will imply a bunch of client requests.).
Philippe
|