View Issue Details

IDProjectCategoryView StatusLast Update
0002701FSSCPtablespublic2012-10-14 19:25
ReporterMjnMixael Assigned ToValathil  
PrioritynormalSeveritycrashReproducibilityalways
Status closedResolutionunable to reproduce 
Product Version3.6.13 
Summary0002701: Missing EFF frame causes FSO to CTD
DescriptionI discovered this while working mainhalls, so that's my only example.

If in the mainhall.tbl you accidentally specify a sound to play on a frame that doesn't exist (say frame 250, but your animation is only 100 frames long).. FSO will CTD.
Steps To ReproduceOpen retail mainhall.tbl and change any of the +Misc Anim Trigger: numbers (except the first one) to some absurd frame number like 500.

I'll be attaching a testcase shortly.
Additional InformationIf FSO needs to terminate, probably best to allow it to do so a little more gracefully and with some useful information attached.
TagsNo tags attached.

Activities

MjnMixael

2012-08-22 00:34

manager   ~0013932

OK, so apparently not. I tried to reproduce with a reliable test case to upload and have been unsuccessful.

Valathil

2012-08-22 14:44

developer   ~0013936

Also very much a case of Garbage In Garbage Out. Not sure if want to fix.

MjnMixael

2012-08-22 15:08

manager   ~0013937

I'm fine with closing this... until I can get something I can upload to reproduce this is likely a waste of time anyway.

The_E

2012-08-22 16:26

administrator  

2701.patch (1,143 bytes)   
Index: code/menuui/mainhallmenu.cpp
===================================================================
--- code/menuui/mainhallmenu.cpp	(revision 9134)
+++ code/menuui/mainhallmenu.cpp	(working copy)
@@ -1972,9 +1972,24 @@
 				required_string("+Misc anim trigger:");
 				int temp_int = 0;
 				stuff_int(&temp_int);
+
+				//load the ani to get the number of frames
+				int num_frames;
+				int ani = bm_load_either(const_cast<char*>(m->misc_anim_name.at(idx).c_str()), &num_frames);
+				//unload it again
+				bm_unload(ani);
+
 				for (s_idx=0; s_idx<temp_int; s_idx++) {
 					m->misc_anim_special_trigger.at(idx).push_back(0);
-					stuff_int(&m->misc_anim_special_trigger.at(idx).at(s_idx));
+					int trigger_frame;
+					stuff_int(&trigger_frame);
+
+					if (trigger_frame < num_frames) {
+						m->misc_anim_special_trigger.at(idx).at(s_idx);
+					} else {
+						WarningEx(LOCATION, "Frame number %d is too high for animation %s. The animation only has %d frames.\nTrigger frame has been set to 0.\n",
+							trigger_frame, m->misc_anim_name.at(idx).c_str(), num_frames);
+					}
 				}
 			}
 
2701.patch (1,143 bytes)   

The_E

2012-08-22 16:26

administrator   ~0013938

Attached a patch for review.

Valathil

2012-08-22 19:13

developer   ~0013940

Nonononononono if you use bm_load_either you load ALL eff frames into memory and then unload it again. Not a very efficient way of getting the framecount.

Valathil

2012-10-14 19:25

developer   ~0013981

Neither me nor Mjn can reproduce. Also analysis of the code in question doesn't imply a crash when large trigger frame numbers are encoutered.

Issue History

Date Modified Username Field Change
2012-08-21 23:53 MjnMixael New Issue
2012-08-22 00:34 MjnMixael Note Added: 0013932
2012-08-22 14:44 Valathil Note Added: 0013936
2012-08-22 15:08 MjnMixael Note Added: 0013937
2012-08-22 16:26 The_E File Added: 2701.patch
2012-08-22 16:26 The_E Note Added: 0013938
2012-08-22 16:49 Echelon9 Status new => code review
2012-08-22 19:13 Valathil Note Added: 0013940
2012-10-14 19:24 Valathil Assigned To => Valathil
2012-10-14 19:24 Valathil Status code review => assigned
2012-10-14 19:25 Valathil Note Added: 0013981
2012-10-14 19:25 Valathil Status assigned => closed
2012-10-14 19:25 Valathil Resolution open => unable to reproduce