View Issue Details

IDProjectCategoryView StatusLast Update
0002875FSSCPHUDpublic2013-11-17 20:32
Reporterniffiwan Assigned ToEchelon9  
PrioritylowSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.19 
Fixed in Version3.7.1 
Summary0002875: AddressSanitizer: alloc-dealloc-mismatch (operator new vs free)
DescriptionReported 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 Reproduce1. 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
TagsNo tags attached.

Activities

Echelon9

2013-05-16 23:33

developer   ~0015061

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()

Echelon9

2013-05-16 23:34

developer  

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();
 }
 
fix-mantis-2875.diff (678 bytes)   

niffiwan

2013-05-17 00:12

developer   ~0015062

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)

Echelon9

2013-05-17 07:02

developer   ~0015063

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?

niffiwan

2013-05-17 09:18

developer   ~0015064

I can't see any #ifdefs in code/globalincs/fsmemory.cpp so I presume it's the same on all platforms.

Echelon9

2013-05-18 05:56

developer   ~0015067

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.

The_E

2013-06-02 08:11

administrator   ~0015113

Let's not be hasty here. It's a legit issue and a good patch for said issue. So it should go in.

Echelon9

2013-06-02 09:08

developer   ~0015114

I'm just not sure that this fix won't lead to memory leaks

niffiwan

2013-11-17 20:31

developer   ~0015413

looks like this was resolved in r10109

Issue History

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