Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

detect mismatched use of malloc/new/new[] vs free/delete/delete[] #123

Closed
derekbruening opened this issue Nov 28, 2014 · 7 comments
Closed

Comments

@derekbruening
Copy link
Contributor

From derek.br...@gmail.com on December 10, 2010 17:57:49

PR 408581

This is not simply a leak issue, which is why it has to be treated
specially: this is an issue of whether or not destructors are called.

Xref the scalar deleting destructor' andvector deleting destructor' used
by the Windows libc.
Note that for objects with no destructor if the code is optimized there may
be no way to distinguish between delete[] and free (no error either to
mismatch but would still be nice to report in case destructor is later
added).

We may want to mark the new[] special header as unaddressable as long as we're
distinguishing new[] from other routes to malloc()

Original issue: http://code.google.com/p/drmemory/issues/detail?id=123

@derekbruening
Copy link
Contributor Author

From bruen...@google.com on October 20, 2011 10:01:43

in my small tests, /O2 leaves the operator delete* in place with their tailcalls, so I have yet to see them optimized away: we'll wait and see whether we see any false positive mismatch errors reported in the field I guess. note that this is much less likely on linux where static libstdc++ is rare.

note that adding new and delete as a first-class heap layer can mask unaddr errors in custom operators. should still catch non-heap accesses (in slowpath) but this will allow operator new and delete to touch any heap header: but since new[] and new
and delete and delete[] should only be allocating memory this seems
reasonable: if you overload them they might do custom allocs and thus can
touch their own headers, and if you don't they should just call malloc/free.

note that the new[] size field is NOT marked unaddressable even with new[] its own heap layer, b/c the caller is the one who asks for extra size and stores the size. we could try to mark the size unaddressable (maybe shift the malloc start too, which would be an alternative to -midchunk_new_ok) but then we have to recognize the code patterns that store it and read it: not worth the effort IMHO.

@derekbruening
Copy link
Contributor Author

From bruen...@google.com on October 20, 2011 18:52:34

an issue with /MD (xref issue #637 ) if don't have symbols: asymmetric inlines!

still goes through cs2bug!operator new[] and cs2bug!operator new and then
msvcr80!operator new (not sure why not in msvcp80), but cs2bug!operator
delete[] goes to msvcr80!operator delete[]: so drmem will raise a mismatch
every single time w/o syms!

0:001> U 00402576
cs2bug!operator new[] [f:\sp\vctools\crt_bld\self_x86\crt\src\newaop.cpp @ 6]:
00402576 e9f9020000 jmp cs2bug!operator new (00402874)
0:001> U 00402874
cs2bug!operator new:
00402874 ff2550b44000 jmp dword ptr [cs2bug!imp??2YAPAXIZ (0040b450)]
0:001> dd 0040b450 L1
0040b450 78160e13
=>
0:000> U 78160e13
MSVCR80!operator new:

0:001> U 00402606
cs2bug!operator delete[]:
00402606 ff2554b44000 jmp dword ptr [cs2bug!_imp??_V@YAXPAX@Z (0040b454)]
0:001> dd 0040b454 L1
0040b454 78160e87
=>
MSVCR80!operator delete[]:
78160e87 e9f1ffffff jmp MSVCR80!operator delete (78160e7d)

this is not just an app issue, where can make it a usage error: could have
random library w/ this problem. in fact this actually happens in cs2bug built /MD (xref issue #636 ) for MSVCP80.dll calling
into MSVCR80.dll, on xp64 where somehow I can't get pdb syms for these SxS
dlls:

Error #8: INVALID HEAP ARGUMENT: wrong free/delete/delete[] called

0 MSVCP80.dll!std::numpunct<wchar_t>::_Tidy

1 MSVCP80.dll!std::numpunct::~numpunct

2 MSVCP80.dll!std::tan

3 MSVCP80.dll!std::_Lockit::_Lockit_dtor

4 MSVCP80.dll!std::locale::_Locimp::_Locimp_dtor

5 MSVCP80.dll!LDunscale

6 MSVCP80.dll!LDunscale

7 MSVCP80.dll!LDunscale

8 ntdll.dll!LdrShutdownProcess

9 KERNEL32.dll!_ExitProcess

#10 KERNEL32.dll!ExitProcess
#11 MSVCR80.dll!amsg_exit
Note: memory was allocated here:
Note: # 0 MSVCP80.dll!std::norm
Note: # 1 MSVCP80.dll!std::numpunct::numpunct
Note: # 2 MSVCP80.dll!std::numpunct::_Getcat
Note: # 3 MSVCP80.dll!std::operator+<unsigned short,std::char_traits,std::allocator >
Note: # 4 MSVCP80.dll!std::num_put<char,std::ostreambuf_iterator<char,std::char_traits > >::_Iput
Note: # 5 MSVCP80.dll!std::num_put<char,std::ostreambuf_iterator<char,std::char_traits > >::do_put
Note: # 6 MSVCP80.dll!std::num_put<wchar_t,std::ostreambuf_iterator<wchar_t,std::char_traits<wchar_t> > >::put
Note: # 7 MSVCP80.dll!std::basic_ostream<char,std::char_traits >::operator<<
Note: # 8 test_leaks [drmemory\git\src\tests\cs2bug.cpp:83]
Note: # 9 main [drmemory\git\src\tests\cs2bug.cpp:133]

Error #9: INVALID HEAP ARGUMENT: wrong free/delete/delete[] called

0 MSVCP80.dll!std::numpunct<wchar_t>::_Tidy

1 MSVCP80.dll!std::numpunct::~numpunct

2 MSVCP80.dll!std::tan

3 MSVCP80.dll!std::_Lockit::_Lockit_dtor

4 MSVCP80.dll!std::locale::_Locimp::_Locimp_dtor

5 MSVCP80.dll!LDunscale

6 MSVCP80.dll!LDunscale

7 MSVCP80.dll!LDunscale

8 ntdll.dll!LdrShutdownProcess

9 KERNEL32.dll!_ExitProcess

#10 KERNEL32.dll!ExitProcess
#11 MSVCR80.dll!amsg_exit
Note: memory was allocated here:
Note: # 0 MSVCP80.dll!std::norm
Note: # 1 MSVCP80.dll!std::numpunct::numpunct
Note: # 2 MSVCP80.dll!std::numpunct::_Getcat
Note: # 3 MSVCP80.dll!std::operator+<unsigned short,std::char_traits,std::allocator >
Note: # 4 MSVCP80.dll!std::num_put<char,std::ostreambuf_iterator<char,std::char_traits > >::_Iput
Note: # 5 MSVCP80.dll!std::num_put<char,std::ostreambuf_iterator<char,std::char_traits > >::do_put
Note: # 6 MSVCP80.dll!std::num_put<wchar_t,std::ostreambuf_iterator<wchar_t,std::char_traits<wchar_t> > >::put
Note: # 7 MSVCP80.dll!std::basic_ostream<char,std::char_traits >::operator<<
Note: # 8 test_leaks [drmemory\git\src\tests\cs2bug.cpp:83]
Note: # 9 main [drmemory\git\src\tests\cs2bug.cpp:133]

Error #10: INVALID HEAP ARGUMENT: wrong free/delete/delete[] called

0 MSVCP80.dll!std::numpunct<wchar_t>::_Tidy

1 MSVCP80.dll!std::numpunct::~numpunct

2 MSVCP80.dll!std::tan

3 MSVCP80.dll!std::_Lockit::_Lockit_dtor

4 MSVCP80.dll!std::locale::_Locimp::_Locimp_dtor

5 MSVCP80.dll!LDunscale

6 MSVCP80.dll!LDunscale

7 MSVCP80.dll!LDunscale

8 ntdll.dll!LdrShutdownProcess

9 KERNEL32.dll!_ExitProcess

#10 KERNEL32.dll!ExitProcess
#11 MSVCR80.dll!amsg_exit
Note: memory was allocated here:
Note: # 0 MSVCP80.dll!std::norm
Note: # 1 MSVCP80.dll!std::numpunct::numpunct
Note: # 2 MSVCP80.dll!std::numpunct::_Getcat
Note: # 3 MSVCP80.dll!std::operator+<unsigned short,std::char_traits,std::allocator >
Note: # 4 MSVCP80.dll!std::num_put<char,std::ostreambuf_iterator<char,std::char_traits > >::_Iput
Note: # 5 MSVCP80.dll!std::num_put<char,std::ostreambuf_iterator<char,std::char_traits > >::do_put
Note: # 6 MSVCP80.dll!std::num_put<wchar_t,std::ostreambuf_iterator<wchar_t,std::char_traits<wchar_t> > >::put
Note: # 7 MSVCP80.dll!std::basic_ostream<char,std::char_traits >::operator<<
Note: # 8 test_leaks [drmemory\git\src\tests\cs2bug.cpp:83]
Note: # 9 main [drmemory\git\src\tests\cs2bug.cpp:133]

on win7 laptop where I can get symbols for easier analysis (getting the
errors b/c haven't downloaded them yet):
Error #8: INVALID HEAP ARGUMENT: wrong free/delete/delete[] called

0 MSVCP90.dll!std::numpunct::_Tidy+0xc (0x7038dcb0 <MSVCP90.dll+0xdcb0>)

1 MSVCP90.dll!std::_Lockit::_Lockit_dtor+0x54 (0x703b5bda <MSVCP90.dll+0x35bda>)

2 MSVCP90.dll!std::locale::_Locimp::_Locimp_dtor+0x87 (0x703b5d6c <MSVCP90.dll+0x35d6c>)

Note: @0:00:00.996 in thread 6176
Note: memory was allocated here:
Note: # 0 MSVCP90.dll!std::norm+0xb4 (0x7038fed9 <MSVCP90.dll+0xfed9>)
Note: # 1 MSVCP90.dll!std::numpunct::_Init+0xc6 (0x70390ec3 <MSVCP90.dll+0x10ec3>)

0:000> U 0x7038dcb0-5
MSVCP90!std::numpunct<wchar_t>::_Tidy+0x8:
7038dcab e82ebb0200 call MSVCP90!operator delete
0:000> U 703b97de
MSVCP90!operator delete[]:
703b97de ff25a8113870 jmp dword ptr [MSVCP90!imp??_VYAXPAXZ (703811a8)]
0:000> dd 703811a8 L1
703811a8 74443f23
0:000> U 74443f23
MSVCR90!operator delete[]:

0:000> U 0x7038fed9 -5
MSVCP90!std::_Maklocstr+0x14:
7038fed4 e856660200 call MSVCP90!operator new
0:000> U 703b652f
MSVCP90!operator new[]:
703b652f 8bff mov edi,edi
703b6531 55 push ebp
703b6532 8bec mov ebp,esp
703b6534 5d pop ebp
703b6535 e9c22f0000 jmp MSVCP90!operator new (703b94fc)
0:000> U 703b94fc
MSVCP90!operator new:
703b94fc ff25c0113870 jmp dword ptr [MSVCP90!imp??2YAPAXIZ (703811c0)]
0:000> U poi(703811c0)
MSVCR90!operator new:

solution?: need to look at caller and only report when have symbols
=> need drsyms query.
wait until symbolize callstack during error report to look at caller?

caller-syms solution: but you could imagine code compiled by some other
compiler that doesn't have local operators, and this is a real error.

simpler idea: don't check caller for syms. instead, assume anyone calling
msvcr* operators has his own copy, so don't report mismatch if 1st layer
(reporting layer) is msvcr*.

risk is if any compiler setting, or other compiler, directly calls msvcr*:
but haven't found one yet.

Owner: bruen...@google.com
Labels: -Priority-Low -Type-Defect Priority-High Type-Enhancement

@derekbruening
Copy link
Contributor Author

From bruen...@google.com on October 21, 2011 11:52:44

another big problem: placement new. even if identify it via arg examination, "placement delete" can vary quite a bit.

better solution: make these operators all be non-adjusting heap layers, only used to check for mismatches. then they don't care whether placement new or not, or what the
operator does in general.

@derekbruening
Copy link
Contributor Author

From derek.br...@gmail.com on October 22, 2011 22:20:55

This issue was closed by revision r564 .

Status: Fixed

@derekbruening
Copy link
Contributor Author

From timurrrr@google.com on October 26, 2011 07:00:25

Usability is not very good, see issue #644

@derekbruening
Copy link
Contributor Author

From timurrrr@google.com on October 26, 2011 07:01:03

Issue 54 has been merged into this issue.

@derekbruening
Copy link
Contributor Author

From bruen...@google.com on October 26, 2011 07:08:43

Usability is not very good, see issue #644 Only on chrome ( issue #643 ) and only when leaks are disabled ( issue #642 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant