View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002875 | FSSCP | HUD | public | 2013-05-16 09:22 | 2013-11-17 20:32 |
| Reporter | niffiwan | Assigned To | Echelon9 | ||
| Priority | low | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.6.19 | ||||
| Fixed in Version | 3.7.1 | ||||
| Summary | 0002875: AddressSanitizer: alloc-dealloc-mismatch (operator new vs free) | ||||
| Description | Reported by AddressSanitizer, a memory error detector for C/C++, in FS2Open builds based on trunk r9677 compiled w/ gcc 4.8.0 ================================================================= ==16772== ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new vs free) on 0x602c00073300 #0 0x7f9581cc848a in free ??:0 0000001 0xde0d9e in _vm_free(void*, char*, int) ??:0 0000002 0x621ac0 in hud_close() /home/me/src/git-fs2_open/code/hud/hud.cpp:1307 0000003 0x41f755 in game_shutdown() /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7263 0000004 0x41f090 in game_main(char*) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7040 0000005 0x41f535 in main /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7168 0000006 0x7f957f76076c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 0x602c00073300 is located 0 bytes inside of 384-byte region [0x602c00073300,0x602c00073480) allocated by thread T0 here: #0 0x7f9581cc495a in operator new(unsigned long) ??:0 0000001 0x696363 in load_gauge_messages(int, int, int, SCP_vector<int>*, color*) /home/me/src/git-fs2_open/code/hud/hudparse.cpp:6300 0000002 0x662bde in load_gauge(int, int, int, int, SCP_vector<int>*, color*) /home/me/src/git-fs2_open/code/hud/hudparse.cpp:871 0000003 0x660905 in load_missing_retail_gauges() /home/me/src/git-fs2_open/code/hud/hudparse.cpp:490 0000004 0x660712 in hud_positions_init() /home/me/src/git-fs2_open/code/hud/hudparse.cpp:467 0000005 0x40f5d1 in game_init() /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:1968 0000006 0x41ef77 in game_main(char*) /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:6977 0000007 0x41f535 in main /home/me/src/git-fs2_open/code/freespace2/freespace.cpp:7168 0000008 0x7f957f76076c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 ==16772== HINT: if you don't care about these warnings you may set ASAN_OPTIONS=alloc_dealloc_mismatch=0 ==16772== ABORTING SCP_vector<HudGauge*> default_hud_gauges; ... ... ... HudGaugeMessages* hud_gauge = new HudGaugeMessages(); ... ... ... } else { default_hud_gauges.push_back(hud_gauge); } and for(j = 0; j < num_gauges; j++) { vm_free(default_hud_gauges[j]); default_hud_gauges[j] = NULL; } | ||||
| Steps To Reproduce | 1. Utilise a version of GCC that supports AddressSantizer (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizer [^]). 2. Build with -fsanitize=address C/C++ flag 3. Run the game & exit after entering mainhall | ||||
| Tags | No tags attached. | ||||
|
|
Thanks for running this. I wasn't picking up the alloc-dealloc-mismatch reports on Clang/OSX as that particular test was recently disabled for my platform. See here: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130422/172468.html One possible solution is to do away with the progressive dealloc of element, and simply vector.clear() the STL container. There is precedent for doing this already on hudparse.cpp:459 The only concern I have with this simplicity approach is that we aren't dealing with the pointers of the STL container. From C++ headers on Mac: /** * Erases all the elements. Note that this function only erases the * elements, and that if the elements themselves are pointers, the * pointed-to memory is not touched in any way. Managing the pointer is * the user's responsibilty. */ void clear() |
|
|
fix-mantis-2875.diff (678 bytes)
Index: hud/hud.cpp
===================================================================
--- hud/hud.cpp (revision 9677)
+++ hud/hud.cpp (working copy)
@@ -1289,24 +1289,11 @@
void hud_close()
{
int i;
- size_t j, num_gauges = 0;
for (i = 0; i < Num_ship_classes; i++) {
- num_gauges = Ship_info[i].hud_gauges.size();
-
- for(j = 0; j < num_gauges; j++) {
- vm_free(Ship_info[i].hud_gauges[j]);
- Ship_info[i].hud_gauges[j] = NULL;
- }
Ship_info[i].hud_gauges.clear();
}
- num_gauges = default_hud_gauges.size();
-
- for(j = 0; j < num_gauges; j++) {
- vm_free(default_hud_gauges[j]);
- default_hud_gauges[j] = NULL;
- }
default_hud_gauges.clear();
}
|
|
|
I *think* that we need to free the memory rather than just clear the vector because the vector is of pointers. If don't do that then we'll have a memory leak? (excepting that we're exiting FSO at this point anyway) Anyway, perhaps replacing the vm_free with delete is what's needed. Of course, we overload delete with a function that just calls vm_free anyway, so it almost seems like a false positive :) void operator delete (void *p) throw() { vm_free(p); } (plus two others) code/globalincs/fsmemory.cpp Similarly new calls vm_malloc_q (same file) |
|
|
That's sounds right that we have to handle the pointers. Overarching point, which I think came through in your post, is that we overload BOTH new and delete to be alloc versions. Is that the case on all platforms? |
|
|
I can't see any #ifdefs in code/globalincs/fsmemory.cpp so I presume it's the same on all platforms. |
|
|
I think we should lower priority on this one, until we can confirm underlying issue and approach to resolve. More pressing bugs to fix for 3.7 at the moment. |
|
|
Let's not be hasty here. It's a legit issue and a good patch for said issue. So it should go in. |
|
|
I'm just not sure that this fix won't lead to memory leaks |
|
|
looks like this was resolved in r10109 |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2013-05-16 09:22 | niffiwan | New Issue | |
| 2013-05-16 09:22 | niffiwan | Status | new => assigned |
| 2013-05-16 09:22 | niffiwan | Assigned To | => niffiwan |
| 2013-05-16 09:23 | niffiwan | Description Updated | |
| 2013-05-16 23:33 | Echelon9 | Note Added: 0015061 | |
| 2013-05-16 23:34 | Echelon9 | File Added: fix-mantis-2875.diff | |
| 2013-05-16 23:34 | Echelon9 | Status | assigned => code review |
| 2013-05-17 00:12 | niffiwan | Note Added: 0015062 | |
| 2013-05-17 07:02 | Echelon9 | Note Added: 0015063 | |
| 2013-05-17 09:18 | niffiwan | Note Added: 0015064 | |
| 2013-05-18 05:56 | Echelon9 | Note Added: 0015067 | |
| 2013-05-31 23:18 | Echelon9 | Priority | normal => low |
| 2013-06-02 08:11 | The_E | Note Added: 0015113 | |
| 2013-06-02 09:08 | Echelon9 | Note Added: 0015114 | |
| 2013-11-17 20:31 | niffiwan | Assigned To | niffiwan => Echelon9 |
| 2013-11-17 20:31 | niffiwan | Status | code review => assigned |
| 2013-11-17 20:31 | niffiwan | Note Added: 0015413 | |
| 2013-11-17 20:32 | niffiwan | Status | assigned => resolved |
| 2013-11-17 20:32 | niffiwan | Fixed in Version | => 3.7.1 |
| 2013-11-17 20:32 | niffiwan | Resolution | open => fixed |
