Source Code Project Mantis - FSSCP
View Issue Details
0002875FSSCPHUDpublic2013-05-16 05:222013-11-17 15:32
Reporterniffiwan 
Assigned ToEchelon9 
PrioritylowSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version3.6.19 
Target VersionFixed 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.
Attached Filesdiff fix-mantis-2875.diff (678) 2013-05-16 19:34
http://scp.indiegames.us/mantis/file_download.php?file_id=2199&type=bug

Notes
(0015061)
Echelon9   
2013-05-16 19:33   
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()
(0015062)
niffiwan   
2013-05-16 20:12   
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)
(0015063)
Echelon9   
2013-05-17 03:02   
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?
(0015064)
niffiwan   
2013-05-17 05:18   
I can't see any #ifdefs in code/globalincs/fsmemory.cpp so I presume it's the same on all platforms.
(0015067)
Echelon9   
2013-05-18 01:56   
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.
(0015113)
The_E   
2013-06-02 04:11   
Let's not be hasty here. It's a legit issue and a good patch for said issue. So it should go in.
(0015114)
Echelon9   
2013-06-02 05:08   
I'm just not sure that this fix won't lead to memory leaks
(0015413)
niffiwan   
2013-11-17 15:31   
looks like this was resolved in r10109

Issue History
2013-05-16 05:22niffiwanNew Issue
2013-05-16 05:22niffiwanStatusnew => assigned
2013-05-16 05:22niffiwanAssigned To => niffiwan
2013-05-16 05:23niffiwanDescription Updatedbug_revision_view_page.php?rev_id=537#r537
2013-05-16 19:33Echelon9Note Added: 0015061
2013-05-16 19:34Echelon9File Added: fix-mantis-2875.diff
2013-05-16 19:34Echelon9Statusassigned => code review
2013-05-16 20:12niffiwanNote Added: 0015062
2013-05-17 03:02Echelon9Note Added: 0015063
2013-05-17 05:18niffiwanNote Added: 0015064
2013-05-18 01:56Echelon9Note Added: 0015067
2013-05-31 19:18Echelon9Prioritynormal => low
2013-06-02 04:11The_ENote Added: 0015113
2013-06-02 05:08Echelon9Note Added: 0015114
2013-11-17 15:31niffiwanAssigned Toniffiwan => Echelon9
2013-11-17 15:31niffiwanStatuscode review => assigned
2013-11-17 15:31niffiwanNote Added: 0015413
2013-11-17 15:32niffiwanStatusassigned => resolved
2013-11-17 15:32niffiwanFixed in Version => 3.7.1
2013-11-17 15:32niffiwanResolutionopen => fixed