|
From: Andrew C. <and...@ci...> - 2011-10-03 14:24:05
|
Hello,
I am using valgrind-3.6.0.SVN-Debian, and cant work out why I am getting
Invalid read errors.
The routine is one which is adding symbols to a std::map, indexed by
their virtual address. As multiple symbols may share the same virtual
address, I need to concatenate the symbol names in the already present
symbol entry.
I have the following (cut down, debugging) code which demonstrates the
problem:
Symbol * prev = prev_itt->second;
char * prev_name = prev->name;
size_t newsize = strlen(prev_name);
newsize += strlen(sym->name);
newsize += 2;
prev->name = new char[newsize];
//Should concat and delete prev_name, but for debugging
leave like this
delete [] prev->name;
//For debugging, put this back, so no leaks
prev->name = prev_name;
With the new char[newsize] and delete [] prev->name commented out,
valgrind is perfectly happy with the code (which is effectively a nop)
However, with the new and delete present as above, valigrind complains with:
==10046== Invalid read of size 1
==10046== at 0x4C286E4: __GI_strlen (mc_replace_strmem.c:284)
==10046== by 0x40CB88: SymbolTable::insert(Symbol*) (symbol-table.cpp:37)
==10046== by 0x406536: parse_symbol_file(std::string) (main.cpp:44)
==10046== by 0x406DA4: main (main.cpp:70)
==10046== Address 0x7ae7d36 is 0 bytes after a block of size 6 alloc'd
==10046== at 0x4C27939: operator new[](unsigned long)
(vg_replace_malloc.c:305)
==10046== by 0x40C969: Symbol::Symbol(unsigned long long, char, char
const*) (symbol.cpp:9)
==10046== by 0x406526: parse_symbol_file(std::string) (main.cpp:44)
==10046== by 0x406DA4: main (main.cpp:70)
==10046==
==10046== Invalid read of size 1
==10046== at 0x4C286E4: __GI_strlen (mc_replace_strmem.c:284)
==10046== by 0x40CB94: SymbolTable::insert(Symbol*) (symbol-table.cpp:38)
==10046== by 0x406536: parse_symbol_file(std::string) (main.cpp:44)
==10046== by 0x406DA4: main (main.cpp:70)
==10046== Address 0x7ae7fe6 is 0 bytes after a block of size 6 alloc'd
==10046== at 0x4C27939: operator new[](unsigned long)
(vg_replace_malloc.c:305)
==10046== by 0x40C969: Symbol::Symbol(unsigned long long, char, char
const*) (symbol.cpp:9)
==10046== by 0x406526: parse_symbol_file(std::string) (main.cpp:44)
==10046== by 0x406DA4: main (main.cpp:70)
Where the two errors are referring to the two strlen() calls when
calculating newsize.
Are these errors indicating a supposed bug in my code, or are they
complaining about something in the __GI_strlen replaced code. If so,
does this mean there is a bug in __GI_strlen ?
Thanks in advance,
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
|
|
From: Tom H. <to...@co...> - 2011-10-03 14:41:44
|
On 03/10/11 15:23, Andrew Cooper wrote: > Where the two errors are referring to the two strlen() calls when > calculating newsize. > > Are these errors indicating a supposed bug in my code, or are they > complaining about something in the __GI_strlen replaced code. If so, > does this mean there is a bug in __GI_strlen ? Most likely it means you are calling strlen on something that isn't nul terminated. Make sure the code at symbol.cpp:9 is nul terminating the string, as that is where the allocation is made that you are running off the end of. Tom -- Tom Hughes (to...@co...) http://compton.nu/ |
|
From: Andrew C. <and...@ci...> - 2011-10-03 15:05:07
|
On 03/10/11 15:41, Tom Hughes wrote: > On 03/10/11 15:23, Andrew Cooper wrote: > >> Where the two errors are referring to the two strlen() calls when >> calculating newsize. >> >> Are these errors indicating a supposed bug in my code, or are they >> complaining about something in the __GI_strlen replaced code. If so, >> does this mean there is a bug in __GI_strlen ? > Most likely it means you are calling strlen on something that isn't nul > terminated. > > Make sure the code at symbol.cpp:9 is nul terminating the string, as > that is where the allocation is made that you are running off the end of. > > Tom > Ah - fantastic catch. Thankyou. I had an off by one error when allocating the original name, which was hidden by a strncpy. FYI: I am in the process of optimizing a working application for space - this bug has come about as a result of converting from std::string to char *. Profiling appears to show this leading to a 7% memory reduction. Is there a useful location to put an FAQ/equiv stating that an apparent error in __GI_strlen might suggest that you are not working with NULL terminating strings? Google was no use which is why I emailed the list. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com |
|
From: Tom H. <to...@co...> - 2011-10-03 15:07:40
|
On 03/10/11 16:04, Andrew Cooper wrote: > Is there a useful location to put an FAQ/equiv stating that an apparent > error in __GI_strlen might suggest that you are not working with NULL > terminating strings? Google was no use which is why I emailed the list. It's kind of valgrind 101 really - read what the error report says and consider what memory that routine will be reading, and where valgrind says that memory was allocated. I mean if valgrind says that strlen is reading 0 bytes beyond the end of a block allocated at a given location then it's a pretty good bet that the nul terminator was missing on a string and it ran off the end. Tom -- Tom Hughes (to...@co...) http://compton.nu/ |
|
From: Andrew C. <and...@ci...> - 2011-10-03 15:19:38
|
On 03/10/11 16:07, Tom Hughes wrote: > On 03/10/11 16:04, Andrew Cooper wrote: > >> Is there a useful location to put an FAQ/equiv stating that an apparent >> error in __GI_strlen might suggest that you are not working with NULL >> terminating strings? Google was no use which is why I emailed the list. > It's kind of valgrind 101 really - read what the error report says and > consider what memory that routine will be reading, and where valgrind > says that memory was allocated. > > I mean if valgrind says that strlen is reading 0 bytes beyond the end of > a block allocated at a given location then it's a pretty good bet that > the nul terminator was missing on a string and it ran off the end. > > Tom I suppose. I am still getting used to valgrind. Anyway, thanks for the help and I will try not to be so dim in the future. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com |
|
From: David C. <dcc...@ac...> - 2011-10-03 16:03:35
|
On 10/3/2011 7:23 AM, Andrew Cooper wrote:
> Hello,
>
> I am using valgrind-3.6.0.SVN-Debian, and cant work out why I am getting
> Invalid read errors.
>
> The routine is one which is adding symbols to a std::map, indexed by
> their virtual address. As multiple symbols may share the same virtual
> address, I need to concatenate the symbol names in the already present
> symbol entry.
>
> I have the following (cut down, debugging) code which demonstrates the
> problem:
>
> Symbol * prev = prev_itt->second;
> char * prev_name = prev->name;
>
> size_t newsize = strlen(prev_name);
> newsize += strlen(sym->name);
> newsize += 2;
>
>
> prev->name = new char[newsize];
> //Should concat and delete prev_name, but for debugging
> leave like this
> delete [] prev->name;
>
> //For debugging, put this back, so no leaks
> prev->name = prev_name;
There is a bug here: prev_name is a copy of a pointer to the memory you
just deleted. prev->name now points to invalid memory. valgrind tries
to keep track of these but if the memory is not referenced for a long
time (and many new allocations), it may not be able to determine that
the reference is to just-freed memory (as opposed to as-yet unallocated
memory).
>
> With the new char[newsize] and delete [] prev->name commented out,
> valgrind is perfectly happy with the code (which is effectively a nop)
It is not a no-op unless those two lines are commented out; see above.
>
> However, with the new and delete present as above, valigrind complains with:
>
> ==10046== Invalid read of size 1
> ==10046== at 0x4C286E4: __GI_strlen (mc_replace_strmem.c:284)
> ==10046== by 0x40CB88: SymbolTable::insert(Symbol*) (symbol-table.cpp:37)
> ==10046== by 0x406536: parse_symbol_file(std::string) (main.cpp:44)
> ==10046== by 0x406DA4: main (main.cpp:70)
> ==10046== Address 0x7ae7d36 is 0 bytes after a block of size 6 alloc'd
> ==10046== at 0x4C27939: operator new[](unsigned long)
> (vg_replace_malloc.c:305)
> ==10046== by 0x40C969: Symbol::Symbol(unsigned long long, char, char
> const*) (symbol.cpp:9)
> ==10046== by 0x406526: parse_symbol_file(std::string) (main.cpp:44)
> ==10046== by 0x406DA4: main (main.cpp:70)
> ==10046==
> ==10046== Invalid read of size 1
> ==10046== at 0x4C286E4: __GI_strlen (mc_replace_strmem.c:284)
> ==10046== by 0x40CB94: SymbolTable::insert(Symbol*) (symbol-table.cpp:38)
> ==10046== by 0x406536: parse_symbol_file(std::string) (main.cpp:44)
> ==10046== by 0x406DA4: main (main.cpp:70)
> ==10046== Address 0x7ae7fe6 is 0 bytes after a block of size 6 alloc'd
> ==10046== at 0x4C27939: operator new[](unsigned long)
> (vg_replace_malloc.c:305)
> ==10046== by 0x40C969: Symbol::Symbol(unsigned long long, char, char
> const*) (symbol.cpp:9)
> ==10046== by 0x406526: parse_symbol_file(std::string) (main.cpp:44)
> ==10046== by 0x406DA4: main (main.cpp:70)
>
> Where the two errors are referring to the two strlen() calls when
> calculating newsize.
>
> Are these errors indicating a supposed bug in my code, or are they
> complaining about something in the __GI_strlen replaced code. If so,
> does this mean there is a bug in __GI_strlen ?
>
> Thanks in advance,
>
--
David Chapman dcc...@ac...
Chapman Consulting -- San Jose, CA
|
|
From: Andrew C. <and...@ci...> - 2011-10-03 15:15:46
|
On 03/10/11 15:52, David Chapman wrote: > On 10/3/2011 7:23 AM, Andrew Cooper wrote: >> Hello, >> >> I am using valgrind-3.6.0.SVN-Debian, and cant work out why I am getting >> Invalid read errors. >> >> The routine is one which is adding symbols to a std::map, indexed by >> their virtual address. As multiple symbols may share the same virtual >> address, I need to concatenate the symbol names in the already present >> symbol entry. >> >> I have the following (cut down, debugging) code which demonstrates the >> problem: >> >> Symbol * prev = prev_itt->second; >> char * prev_name = prev->name; >> >> size_t newsize = strlen(prev_name); >> newsize += strlen(sym->name); >> newsize += 2; >> >> >> prev->name = new char[newsize]; >> //Should concat and delete prev_name, but for debugging >> leave like this >> delete [] prev->name; >> >> //For debugging, put this back, so no leaks >> prev->name = prev_name; > There is a bug here: prev_name is a copy of a pointer to the memory you > just deleted. prev->name now points to invalid memory. valgrind tries > to keep track of these but if the memory is not referenced for a long > time (and many new allocations), it may not be able to determine that > the reference is to just-freed memory (as opposed to as-yet unallocated > memory). prev_name is a copy of the original prev->name. However, the next thing which happens is to prev->name is pointed to a new heap location, without deleting the original. Now prev_name and prev->name point to different, valid heap locations. For debugging purposes, I then deleted that new heap location, turning prev->name into an invalid pointer. Finally, assigned prev_name (which is still valid) back to prev->name. Therefore it is a nop, whether the new/delete are present or not. Anyway - Tom Huges already identified the issue and valgrind is now happy with my proper concat code. >> With the new char[newsize] and delete [] prev->name commented out, >> valgrind is perfectly happy with the code (which is effectively a nop) > It is not a no-op unless those two lines are commented out; see above. > >> However, with the new and delete present as above, valigrind complains with: >> >> ==10046== Invalid read of size 1 >> ==10046== at 0x4C286E4: __GI_strlen (mc_replace_strmem.c:284) >> ==10046== by 0x40CB88: SymbolTable::insert(Symbol*) (symbol-table.cpp:37) >> ==10046== by 0x406536: parse_symbol_file(std::string) (main.cpp:44) >> ==10046== by 0x406DA4: main (main.cpp:70) >> ==10046== Address 0x7ae7d36 is 0 bytes after a block of size 6 alloc'd >> ==10046== at 0x4C27939: operator new[](unsigned long) >> (vg_replace_malloc.c:305) >> ==10046== by 0x40C969: Symbol::Symbol(unsigned long long, char, char >> const*) (symbol.cpp:9) >> ==10046== by 0x406526: parse_symbol_file(std::string) (main.cpp:44) >> ==10046== by 0x406DA4: main (main.cpp:70) >> ==10046== >> ==10046== Invalid read of size 1 >> ==10046== at 0x4C286E4: __GI_strlen (mc_replace_strmem.c:284) >> ==10046== by 0x40CB94: SymbolTable::insert(Symbol*) (symbol-table.cpp:38) >> ==10046== by 0x406536: parse_symbol_file(std::string) (main.cpp:44) >> ==10046== by 0x406DA4: main (main.cpp:70) >> ==10046== Address 0x7ae7fe6 is 0 bytes after a block of size 6 alloc'd >> ==10046== at 0x4C27939: operator new[](unsigned long) >> (vg_replace_malloc.c:305) >> ==10046== by 0x40C969: Symbol::Symbol(unsigned long long, char, char >> const*) (symbol.cpp:9) >> ==10046== by 0x406526: parse_symbol_file(std::string) (main.cpp:44) >> ==10046== by 0x406DA4: main (main.cpp:70) >> >> Where the two errors are referring to the two strlen() calls when >> calculating newsize. >> >> Are these errors indicating a supposed bug in my code, or are they >> complaining about something in the __GI_strlen replaced code. If so, >> does this mean there is a bug in __GI_strlen ? >> >> Thanks in advance, >> > -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com |