2019-12-06 05:46 EST


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0002875FSSCPHUDpublic2013-11-17 15:32
Reporterniffiwan 
Assigned ToEchelon9 
PrioritylowSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
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 Files
  • diff file icon fix-mantis-2875.diff (678 bytes) 2013-05-16 19:34 -
    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();
     }
     
    
    diff file icon fix-mantis-2875.diff (678 bytes) 2013-05-16 19:34 +

-Relationships
+Relationships

-Notes

~0015061

Echelon9 (developer)

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

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

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

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

~0015067

Echelon9 (developer)

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

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

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

~0015413

niffiwan (developer)

looks like this was resolved in r10109
+Notes

-Issue History
Date Modified Username Field Change
2013-05-16 05:22 niffiwan New Issue
2013-05-16 05:22 niffiwan Status new => assigned
2013-05-16 05:22 niffiwan Assigned To => niffiwan
2013-05-16 05:23 niffiwan Description Updated View Revisions
2013-05-16 19:33 Echelon9 Note Added: 0015061
2013-05-16 19:34 Echelon9 File Added: fix-mantis-2875.diff
2013-05-16 19:34 Echelon9 Status assigned => code review
2013-05-16 20:12 niffiwan Note Added: 0015062
2013-05-17 03:02 Echelon9 Note Added: 0015063
2013-05-17 05:18 niffiwan Note Added: 0015064
2013-05-18 01:56 Echelon9 Note Added: 0015067
2013-05-31 19:18 Echelon9 Priority normal => low
2013-06-02 04:11 The_E Note Added: 0015113
2013-06-02 05:08 Echelon9 Note Added: 0015114
2013-11-17 15:31 niffiwan Assigned To niffiwan => Echelon9
2013-11-17 15:31 niffiwan Status code review => assigned
2013-11-17 15:31 niffiwan Note Added: 0015413
2013-11-17 15:32 niffiwan Status assigned => resolved
2013-11-17 15:32 niffiwan Fixed in Version => 3.7.1
2013-11-17 15:32 niffiwan Resolution open => fixed
+Issue History