View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002852 | FSSCP | public | 2013-04-26 09:25 | 2013-06-22 02:59 | |
| Reporter | Echelon9 | Assigned To | Goober5000 | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.6.19 | ||||
| Target Version | 3.7.0 | ||||
| Summary | 0002852: AddressSanitizer: global-buffer-overflow in shockwave_move() shockwave.cpp:359 | ||||
| Description | Reported by AddressSanitizer, a memory error detector for C/C++, in FS2Open builds based on trunk r9646. ERROR: AddressSanitizer: global-buffer-overflow on address 0x000104a00fbc at pc 0x1025afe24 bp 0x7fff5fbfc490 sp 0x7fff5fbfc488 READ of size 4 at 0x000104a00fbc thread T0 #0 0x1025afe23 in shockwave_move shockwave.cpp:359 0000001 0x1025b402e in shockwave_move_all shockwave.cpp:625 0000002 0x1002efbbb in game_simulation_frame freespace.cpp:4037 0000003 0x1002f4440 in game_frame freespace.cpp:4380 0000004 0x1002f9c5b in game_do_frame freespace.cpp:4791 0000005 0x100305796 in game_do_state freespace.cpp:6467 0000006 0x100900d68 in gameseq_process_events gamesequence.cpp:405 0000007 0x10030c026 in game_main freespace.cpp:7034 0000008 0x10030d6c4 in SDL_main freespace.cpp:7168 ... /** * Simulate a single shockwave. If the shockwave radius exceeds outer_radius, then * delete the shockwave. * * @param shockwave_objp object pointer that points to shockwave object * @param frametime time to simulate shockwave */ void shockwave_move(object *shockwave_objp, float frametime) { shockwave *sw; ... // If this shockwave hit the player, play shockwave impact sound if ( objp == Player_obj ) { snd_play( &Snds[SND_SHOCKWAVE_IMPACT], 0.0f, MAX(0.4f, damage/Weapon_info[sw->weapon_info_index].damage) ); <==== } } // end for } | ||||
| Steps To Reproduce | 1. Utilise a version of Clang that supports AddressSantizer (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizer). 2. Build with -fsanitize=address C/C++ flag 3. Run the game with mediavps 3.6.12, destroy a capital ship and be reasonably nearby | ||||
| Additional Information | ERROR: AddressSanitizer: global-buffer-overflow on address 0x000104a00fbc at pc 0x1025afe24 bp 0x7fff5fbfc490 sp 0x7fff5fbfc488 READ of size 4 at 0x000104a00fbc thread T0 #0 0x1025afe23 in shockwave_move shockwave.cpp:359 0000001 0x1025b402e in shockwave_move_all shockwave.cpp:625 0000002 0x1002efbbb in game_simulation_frame freespace.cpp:4037 0000003 0x1002f4440 in game_frame freespace.cpp:4380 0000004 0x1002f9c5b in game_do_frame freespace.cpp:4791 0000005 0x100305796 in game_do_state freespace.cpp:6467 0000006 0x100900d68 in gameseq_process_events gamesequence.cpp:405 0000007 0x10030c026 in game_main freespace.cpp:7034 0000008 0x10030d6c4 in SDL_main freespace.cpp:7168 0000009 0x100003223 in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300 0000010 0x7fff8e0f0ed9 in _CFXNotificationPost (in CoreFoundation) + 2553 #11 0x7fff9019ee25 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63 0000012 0x7fff8ab3355c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291 0000013 0x7fff8ab33295 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215 0000014 0x7fff8ab30481 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565 0000015 0x7fff8ab3007b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350 0000016 0x7fff901b870a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307 0000017 0x7fff901b856c in _NSAppleEventManagerGenericHandler (in Foundation) + 105 0000018 0x7fff8f100077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306 0000019 0x7fff8f0ffed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36 0000020 0x7fff8f0ffd98 in aeProcessAppleEvent (in AE) + 317 0000021 0x7fff8d126708 in AEProcessAppleEvent (in HIToolbox) + 99 0000022 0x7fff8ab2c865 in _DPSNextEvent (in AppKit) + 1455 0000023 0x7fff8ab2be21 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127 0000024 0x7fff8ab231d2 in -[NSApplication run] (in AppKit) + 516 0000025 0x100005382 in CustomApplicationMain SDLMain.m:227 0000026 0x100004def in main SDLMain.m:377 0000027 0x100001f43 in start (in FS2_Open (debug)) + 51 0000028 0x0 in 0x0 0x000104a00fbc is located 258780 bytes to the right of global variable 'Weapons' from 'trunk/fs2_open/projects/Xcode4/../../code/weapon/weapons.cpp' (0x104879ae0) of size 1344000 Shadow bytes around the buggy address: 0x1000209401a0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209401b0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209401c0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209401d0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209401e0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 =>0x1000209401f0: f9 f9 f9 f9 f9 f9 f9[f9]f9 f9 f9 f9 f9 f9 f9 f9 0x100020940200: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020940210: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020940220: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020940230: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020940240: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 ASan internal: fe ==22670==ABORTING | ||||
| Tags | No tags attached. | ||||
|
|
Where a shockwave is caused by a ship explosion, the shockwave information is not setup to scale the sound volume by maximum potential damage from a given weapon. |
|
|
Fix committed to trunk@9695. |
|
|
Goober, your changes in r9696 have reversed the logic of the volume in a particular case. It will cap the volume of near explosions to only 40% in all cases. Let me explain: Consider the case where a shockwave is not caused by a weapon, say instead an exploding ship. There are two important ranges of values here: (a) where damage from that shockwave is >40% and (b) where damage from that shockwave is <40%. The intention of the old code, and the effect of my r9695, would mean: (a) damage >40% -> volume is equal to that damage percentage (b) damage <40% -> volume is exactly 40% Your new code in r9696 leads to the following outcomes: (a) damage >40% -> volume is exactly 40% (b) damage <40% -> volume is exactly 40% Do you agree the attached, revised, patch against trunk resolves the issue to both our satisfactions? |
|
|
revised-mantis2852.patch (801 bytes)
Index: code/weapon/shockwave.cpp
===================================================================
--- code/weapon/shockwave.cpp (revision 9696)
+++ code/weapon/shockwave.cpp (working copy)
@@ -356,13 +356,11 @@
// If this shockwave hit the player, play shockwave impact sound
if ( objp == Player_obj ) {
- float vol_scale;
+ float vol_scale = damage;
if (sw->weapon_info_index >= 0 && Weapon_info[sw->weapon_info_index].damage != 0.0f) {
- vol_scale = MAX(0.4f, damage/Weapon_info[sw->weapon_info_index].damage);
- } else {
- vol_scale = 0.4f;
+ vol_scale = damage/Weapon_info[sw->weapon_info_index].damage;
}
- snd_play( &Snds[SND_SHOCKWAVE_IMPACT], 0.0f, vol_scale );
+ snd_play( &Snds[SND_SHOCKWAVE_IMPACT], 0.0f, MAX(0.4f, vol_scale) );
}
} // end for
|
|
|
I can't see how you came to this conclusion: --- Your new code in r9696 leads to the following outcomes: (a) damage >40% -> volume is exactly 40% (b) damage <40% -> volume is exactly 40% --- For most cases, the volume scale calculation is exactly the way it was in retail, or prior to your commit. It's only in the case where a weapon doesn't exist (or does zero damage) that it's different. I set the value to 40%, but that's just a default value that can be changed. But I'm not opposed to changing it to some other default value. In fact, it probably should be 100%. I've attached a new patch that specifically takes into account shockwave damage that doesn't come from a weapon. See what you think. |
|
|
shockwave.cpp.patch (907 bytes)
Index: code/weapon/shockwave.cpp
===================================================================
--- code/weapon/shockwave.cpp (revision 9696)
+++ code/weapon/shockwave.cpp (working copy)
@@ -356,12 +356,17 @@
// If this shockwave hit the player, play shockwave impact sound
if ( objp == Player_obj ) {
- float vol_scale;
- if (sw->weapon_info_index >= 0 && Weapon_info[sw->weapon_info_index].damage != 0.0f) {
- vol_scale = MAX(0.4f, damage/Weapon_info[sw->weapon_info_index].damage);
+ float full_damage, vol_scale;
+ if (sw->weapon_info_index >= 0) {
+ full_damage = Weapon_info[sw->weapon_info_index].damage;
} else {
- vol_scale = 0.4f;
+ full_damage = sw->damage;
}
+ if (full_damage != 0.0f) {
+ vol_scale = MAX(0.4f, damage/full_damage);
+ } else {
+ vol_scale = 1.0f;
+ }
snd_play( &Snds[SND_SHOCKWAVE_IMPACT], 0.0f, vol_scale );
}
|
|
|
Goober, To quote directly from you: "For most cases, the volume scale calculation is exactly the way it was in retail, or prior to your commit. It's only in the case where a weapon doesn't exist (or does zero damage) that it's different." That is exactly the issue, your commit to trunk and proposed changes are needlessly changing retail behaviour. I pretty clearly showed why your code is different to retail. The point isn't what particular value you chose, be that 40% or 100%, but that you hardcode a particular value. Retail and my code can dynamically set the audio volume based on the damage experienced to player's craft by an exploding ship. My commit to trunk both (i) preserves retail behaviour, and (ii) addresses the memory corruption issue that this Mantis report relates to. I don't understand why you want to come up with different patches that don't meet those two tests? |
|
|
I think you're mistaken. First of all, you didn't clearly show why my code was different from retail. Although you listed a breakdown of scale values, you didn't show how my code allegedly got to those values. I took another careful look at what I had written and didn't see how that could be the case. Second of all, your code doesn't follow the retail contract. The volume scale is supposed to be a value from 0.0 to 1.0, yet you assign it the damage variable. In most cases, damage will be greater than 1.0, so this throws the scaling entirely out of whack. It helps to understand why the code was written. In the retail calculation of the volume scale, the shockwave volume is scaled to the level of damage the craft receives compared to the level of damage the weapon is capable of doing. The MAX function ensures that this proportion is always at least 40%. If you assign the vol_scale variable as the damage without dividing it by anything, it no longer becomes a proportion. (Or more accurately, it becomes a ridiculously large proportion.) |
|
|
Resolving, with the latest version of the patch, since discussion seems to have run its course. |
|
fs2open: trunk r9695 2013-06-14 21:15 Ported: N/A Details Diff |
Fix Mantis 2852: AddressSanitizer: global-buffer-overflow in shockwave_move() shockwave.cpp:359 |
Affected Issues 0002852 |
|
| mod - /trunk/fs2_open/code/weapon/shockwave.cpp | Diff File | ||
|
fs2open: trunk r9696 2013-06-14 22:21 Ported: N/A Details Diff |
Mantis 0002852: good catch, but I'd rather not modify the damage variable... and also, there's the danger of divide-by-zero which wasn't checked |
Affected Issues 0002852 |
|
| mod - /trunk/fs2_open/code/weapon/shockwave.cpp | Diff File | ||
|
fs2open: trunk r9697 2013-06-22 00:00 Ported: N/A Details Diff |
Mantis 0002852: commit the newest version of the patch, based on discussion |
Affected Issues 0002852 |
|
| mod - /trunk/fs2_open/code/weapon/shockwave.cpp | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2013-04-26 09:25 | Echelon9 | New Issue | |
| 2013-04-26 09:25 | Echelon9 | Status | new => assigned |
| 2013-04-26 09:25 | Echelon9 | Assigned To | => Echelon9 |
| 2013-06-15 00:14 | Echelon9 | Note Added: 0015124 | |
| 2013-06-15 00:15 | Echelon9 | Changeset attached | => fs2open trunk r9695 |
| 2013-06-15 00:15 | Echelon9 | Note Added: 0015125 | |
| 2013-06-15 00:15 | Echelon9 | Status | assigned => resolved |
| 2013-06-15 00:15 | Echelon9 | Resolution | open => fixed |
| 2013-06-15 01:21 | Goober5000 | Changeset attached | => fs2open trunk r9696 |
| 2013-06-16 00:36 | Echelon9 | Assigned To | Echelon9 => Goober5000 |
| 2013-06-16 00:36 | Echelon9 | Note Added: 0015127 | |
| 2013-06-16 00:36 | Echelon9 | Status | resolved => feedback |
| 2013-06-16 00:36 | Echelon9 | Resolution | fixed => reopened |
| 2013-06-16 00:37 | Echelon9 | File Added: revised-mantis2852.patch | |
| 2013-06-16 04:48 | Goober5000 | Note Added: 0015128 | |
| 2013-06-16 04:49 | Goober5000 | File Added: shockwave.cpp.patch | |
| 2013-06-16 11:10 | Echelon9 | Note Added: 0015129 | |
| 2013-06-16 11:10 | Echelon9 | Status | feedback => assigned |
| 2013-06-16 16:16 | Goober5000 | Note Added: 0015130 | |
| 2013-06-22 02:58 | Goober5000 | Changeset attached | => fs2open trunk r9697 |
| 2013-06-22 02:59 | Goober5000 | Note Added: 0015133 | |
| 2013-06-22 02:59 | Goober5000 | Status | assigned => resolved |
| 2013-06-22 02:59 | Goober5000 | Resolution | reopened => fixed |
