View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002834 | FSSCP | public | 2013-04-03 12:13 | 2013-04-22 11:21 | |
Reporter | Echelon9 | Assigned To | Echelon9 | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.6.19 | ||||
Target Version | 3.7.0 | ||||
Summary | 0002834: AddressSanitizer: global-buffer-overflow in get_max_ammo_count_for_bank() ship.cpp:15249 | ||||
Description | Reported by AddressSanitizer, a memory error detector for C/C++, in FS2Open builds based on trunk r9617. ERROR: AddressSanitizer: global-buffer-overflow on address 0x0001049b1c40 at pc 0x1020be74a bp 0x7fff5fbfb230 sp 0x7fff5fbfb228 READ of size 4 at 0x0001049b1c40 thread T0 #0 0x1020be749 in get_max_ammo_count_for_bank ship.cpp:15249 0000001 0x1020baa3a in ship_process_post ship.cpp:8198 0000002 0x101a5a5ca in obj_move_all_post object.cpp:1226 0000003 0x101a5e6f2 in obj_move_all object.cpp:1445 0000004 0x1002ea2fd in game_simulation_frame freespace.cpp:3987 0000005 0x1002eef51 in game_frame freespace.cpp:4380 0000006 0x1002f473b in game_do_frame freespace.cpp:4791 0000007 0x10030029a in game_do_state freespace.cpp:6466 0000008 0x1008f1b89 in gameseq_process_events gamesequence.cpp:405 0000009 0x100306b18 in game_main freespace.cpp:7033 0000010 0x100308186 in SDL_main freespace.cpp:7167 ... /** * Determine the number of secondary ammo units (missile/bomb) allowed max for a ship */ int get_max_ammo_count_for_bank(int ship_class, int bank, int ammo_type) { float capacity, size; capacity = (float) Ship_info[ship_class].secondary_bank_ammo_capacity[bank]; size = (float) Weapon_info[ammo_type].cargo_size; <======== return (int) (capacity / size); } | ||||
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, progress through to flight in red alert mission 'Apocalypse' | ||||
Additional Information | ERROR: AddressSanitizer: global-buffer-overflow on address 0x0001049b1c40 at pc 0x1020be74a bp 0x7fff5fbfb230 sp 0x7fff5fbfb228 READ of size 4 at 0x0001049b1c40 thread T0 #0 0x1020be749 in get_max_ammo_count_for_bank ship.cpp:15249 0000001 0x1020baa3a in ship_process_post ship.cpp:8198 0000002 0x101a5a5ca in obj_move_all_post object.cpp:1226 0000003 0x101a5e6f2 in obj_move_all object.cpp:1445 0000004 0x1002ea2fd in game_simulation_frame freespace.cpp:3987 0000005 0x1002eef51 in game_frame freespace.cpp:4380 0000006 0x1002f473b in game_do_frame freespace.cpp:4791 0000007 0x10030029a in game_do_state freespace.cpp:6466 0000008 0x1008f1b89 in gameseq_process_events gamesequence.cpp:405 0000009 0x100306b18 in game_main freespace.cpp:7033 0000010 0x100308186 in SDL_main freespace.cpp:7167 #11 0x10000287a in -[SDLMain applicationDidFinishLaunching:] SDLMain.m:300 0000012 0x7fff8ffe3ed9 in _CFXNotificationPost (in CoreFoundation) + 2553 0000013 0x7fff85fc6e25 in -[NSNotificationCenter postNotificationName:object:userInfo:] (in Foundation) + 63 0000014 0x7fff8b92855c in -[NSApplication _postDidFinishNotification] (in AppKit) + 291 0000015 0x7fff8b928295 in -[NSApplication _sendFinishLaunchingNotification] (in AppKit) + 215 0000016 0x7fff8b925481 in -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] (in AppKit) + 565 0000017 0x7fff8b92507b in -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] (in AppKit) + 350 0000018 0x7fff85fe070a in -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] (in Foundation) + 307 0000019 0x7fff85fe056c in _NSAppleEventManagerGenericHandler (in Foundation) + 105 0000020 0x7fff8e3a9077 in aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) (in AE) + 306 0000021 0x7fff8e3a8ed8 in dispatchEventAndSendReply(AEDesc const*, AEDesc*) (in AE) + 36 0000022 0x7fff8e3a8d98 in aeProcessAppleEvent (in AE) + 317 0000023 0x7fff88b11708 in AEProcessAppleEvent (in HIToolbox) + 99 0000024 0x7fff8b921865 in _DPSNextEvent (in AppKit) + 1455 0000025 0x7fff8b920e21 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 127 0000026 0x7fff8b9181d2 in -[NSApplication run] (in AppKit) + 516 0000027 0x100004993 in CustomApplicationMain SDLMain.m:227 0000028 0x100004410 in main SDLMain.m:377 0000029 0x1000015e3 in start (in FS2_Open (debug)) + 51 0000030 0x0 in 0x0 0x0001049b1c40 is located 259072 bytes to the right of global variable 'Weapons' from 'fs2open/trunk/fs2_open/projects/Xcode4/../../code/weapon/weapons.cpp' (0x10482a640) of size 1344000 Shadow bytes around the buggy address: 0x100020936330: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020936340: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020936350: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020936360: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x100020936370: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 =>0x100020936380: f9 f9 f9 f9 f9 f9 f9 f9[f9]f9 f9 f9 f9 f9 f9 f9 0x100020936390: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209363a0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209363b0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209363c0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 0x1000209363d0: 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 righ 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 ==89431==ABORTING | ||||
Tags | No tags attached. | ||||
|
fix-mantis-2834_get_max_ammo_count_for_bank.patch (715 bytes)
Index: ship/ship.cpp =================================================================== --- ship/ship.cpp (revision 9638) +++ ship/ship.cpp (working copy) @@ -15244,10 +15251,14 @@ int get_max_ammo_count_for_bank(int ship_class, int bank, int ammo_type) { float capacity, size; - - capacity = (float) Ship_info[ship_class].secondary_bank_ammo_capacity[bank]; - size = (float) Weapon_info[ammo_type].cargo_size; - return (int) (capacity / size); + + if (ship_class < 0 || bank < 0 || ammo_type < 0) { + return 0; + } else { + capacity = (float) Ship_info[ship_class].secondary_bank_ammo_capacity[bank]; + size = (float) Weapon_info[ammo_type].cargo_size; + return (int) (capacity / size); + } } /** |
|
Ready for code review. This was another case of -1 being passed as an array index. |
|
looks good to me |
|
Fix committed to trunk@9645. |
Date Modified | Username | Field | Change |
---|---|---|---|
2013-04-03 12:13 | Echelon9 | New Issue | |
2013-04-03 12:15 | Echelon9 | Relationship added | related to 0002821 |
2013-04-13 12:48 | Echelon9 | Assigned To | => Echelon9 |
2013-04-13 12:48 | Echelon9 | Status | new => assigned |
2013-04-19 10:27 | Echelon9 | File Added: fix-mantis-2834_get_max_ammo_count_for_bank.patch | |
2013-04-19 10:28 | Echelon9 | Note Added: 0014933 | |
2013-04-19 10:28 | Echelon9 | Assigned To | Echelon9 => chief1983 |
2013-04-19 10:28 | Echelon9 | Status | assigned => code review |
2013-04-22 06:04 | niffiwan | Note Added: 0014947 | |
2013-04-22 11:21 | Echelon9 | Assigned To | chief1983 => Echelon9 |
2013-04-22 11:21 | Echelon9 | Status | code review => assigned |
2013-04-22 11:21 | Echelon9 | Changeset attached | => fs2open trunk r9645 |
2013-04-22 11:21 | Echelon9 | Note Added: 0014951 | |
2013-04-22 11:21 | Echelon9 | Status | assigned => resolved |
2013-04-22 11:21 | Echelon9 | Resolution | open => fixed |