Home » MODDING HQ 1.13 » v1.13 Bug Reports » Horrible Encyclopedia code crashes the game
Horrible Encyclopedia code crashes the game[message #345928] Fri, 17 June 2016 01:59 Go to next message
Flugente

 
Messages:3509
Registered:April 2009
Location: Germany
While currently developing a new feature, a most bizarre error occured - all of a sudden, the game cannot be started, as it crashes during initialisation. The error is located in void GameInitEncyclopediaData_NEW( ), which is quite odd, as I didn't change anything even remotely related to the encyclopedia feature. To be precise, the error happens here:
/**
* Checks for all needed files (graphics, xmls, ini option).
*
* Called when a new game is started (guiCurrentScreen == 0), entering new game screen/loading game from main menu (guiCurrentScreen == MAINMENU_SCREEN) and quitting game (guiCurrentScreen == MSG_BOX_SCREEN).
* Sets gui element initial values.
* Shows error message and deactivates the Encyclopedia if a file is missing.
* @todo check for all files needed by data page here.
*/
void GameInitEncyclopediaData_NEW(  )
{
	//initialize UI elements
	memset( giEncyclopedia_DataBtn, BUTTON_NO_SLOT, sizeof(giEncyclopedia_DataBtn) );
	giEncyclopedia_DataBtnImage = BUTTON_NO_IMAGE;
	memset( giEncyclopedia_DataFilterBtn, BUTTON_NO_SLOT, sizeof(giEncyclopedia_DataFilterBtn) );
	giEncyclopedia_DataFilterBtnImage = BUTTON_NO_IMAGE;
#if 0//debug
	memset( gbEncyclopediaData_ItemVisible, ENC_ITEM_DISCOVERED_NOT_REACHABLE, sizeof(gbEncyclopediaData_ItemVisible) );
	gbEncyclopediaData_ItemVisible[1] = ENC_ITEM_NOT_DISCOVERED;
#else
	if( guiCurrentScreen == MAINMENU_SCREEN )
		EncyclopediaInitItemsVisibility();
#endif
	// do following only once at start of JA2
	CHECKV( guiCurrentScreen == 0 );
	//prepare indexes for subfilter texts defined in _LanguageText.cpp, assuming there are blank separators between filter button texts ("1", "2", "3", "", "1", "", "1", "2", "3", "4")
	STR16 *pText, testStr;
	UINT8 failsafe = 1, index;
	for( UINT8 site = 1; site <= ENC_NUM_SUBPAGES; site++ )
	{
		//get text pointer
		switch(site)
		{
		case ENC_LOCATIONS :
			pText = pEncyclopediaSubFilterLocationText;
			break;
		case ENC_CHARACTERS :
			pText = pEncyclopediaSubFilterCharText;
			break;
		case ENC_ITEMS :
			pText = pEncyclopediaSubFilterItemText;
			break;
		case ENC_QUESTS :
			pText = pEncyclopediaSubFilterQuestText;
			break;
		default : 
			AssertMsg(0, "Unrecognized DataPage in GameInitEncyclopediaData()");
			return;
		}
		index = 0;
		for( UINT8 filter = 0; filter < ENC_DATA_MAX_FILTERS-1; filter++ )
			if(filter == 0) guiEncyclopediaSubFilterTextIndexStart[site-1][filter] = 0;
			else
			{
				testStr = pText[index],failsafe = 1;
				while( failsafe && testStr[0] != L'\0' ) testStr = pText[++index], failsafe++;			[b]<--- this is where the crash happens[/b]
				guiEncyclopediaSubFilterTextIndexStart[site-1][filter] = ++index;
			}
	}
}


In the line while( failsafe && testStr[0] != L'\0' ) testStr = pText[++index], failsafe++;, testStr[0] is a NULL-pointer, which causes the game to crash.

This is rather intriguing - this part hasn't been changed in ages, so why does it crash now?

Because whatever this code is supposed to do, it is fundamentally broken, that's why.

pText refers to one of the string arrays (pEncyclopediaSubFilterQuestText etc.) from above. guiEncyclopediaSubFilterTextIndexStart is then supposed to be filled with an index to... something. For that we loop over the strings refferred to via pText and test them each for being emtpy (testStr[0] != L'\0').

This is very bad on several levels.
First, we never check for NULL-pointers. This means we happily crash when we get one.
Second (and much, MUCH worse) is that we never check for the end of an array. pEncyclopediaSubFilterQuestText etc. are of limited size, but we continue looping even if we reach their end.
THIS MEANS THAT WE ACCESS WHATEVER IS IN MEMORY NEXT AND INTERPRET THAT AS A STRING! This is doubleplusbad. What comes next in memory pretty much relies on the whims of your compiler. Most of the time you will simply loop over string arrays in _EnglishText.cpp until you find an empty string, then assign whatever array you have to your function.
As a result, whatever entries guiEncyclopediaSubFilterTextIndexStart has is more or less random.
This means that whatever the intention of this code, the enclyopedia is pretty much broken here.
As nobody has (to my knowledge) ever complained about this, this also indicates that encyclopedia is both broken and not used by anybody.

Also note that as failsafe and index are UINT8 and never checked against overflow, they will happily overflow if you loop long enough.

Now, what apparently happened to me here is that some part of my new code is apparently assigned to be close after one of those string arrays. It then gets used, the code finds a NULL pointer, and the game crashes, jsut because the encyclopedia code doesn't like random bits of code unrelated to it in other locations.

To test that premise, I checked out a clean repository of the current trunk, then added the following array in a specific location:
STR16 pEncyclopediaSubFilterQuestText[] =
{//Quest subfilter button text max 7 chars, not used, but needed if any subfilters are added
//..L"------v"
	L"",//reserved. insert new active quest subfilters above!
	L"",//reserved. insert new completed quest subfilters above!
};

STR16	FlugentesArrayofFun[]=
{
	NULL,
	L"Bla",
	L"Bla",
        ... this goes on a while, exact size is machine- and compiler-dependant ....
	L"Bla",
	L"",
};

Remove the NULL, and you can pretty much control what entries guiEncyclopediaSubFilterTextIndexStart gets by altering that array (note that as the compiler determines how arrays are stored internally, this is likely machine-dependant - this part works for me).
Otherwise, the game will simply crash once it hits the NULL-pointer.

This is beyond broken. This array isn't used anywhere in the code. The encyclopedia randomly accesses memory. This is terrible.

And to be perfectly blunt and honest, I've had it with certain persons code. As I frequently state, erronous code is okay - everybody does errors. But this is undocumented, unsafe and doesn't seem to work in the first place.

Either the people responsible for encyclopedia code fix this themselves (whether that is Jazz, Moa or Rowa is of no interest to me), or I will do that by liberally deleting that feature. If you have a problem with that, state that, and I will gladly move to a personal branch where I dont have to deal with this and similar issues.



I know now that it could never work between us, as much as we wanted to, it could never be! Not because you're a rabbit, but because you're black.

If you want, you can donate to me. This will not affect how and what I code, and I will not code specific features in return. I will be thankful though.

Report message to a moderator

Captain

Re: Horrible Encyclopedia code crashes the game[message #345929 is a reply to message #345928] Fri, 17 June 2016 07:13 Go to previous messageGo to next message
wanne (aka RoWa21) is currently offline wanne (aka RoWa21)

 
Messages:1961
Registered:October 2005
Location: Austria
Hi Flugente,

I will forward your post to Jazz so he can fix his code instead of removing the feature.

Report message to a moderator

Sergeant Major

Re: Horrible Encyclopedia code crashes the game[message #345930 is a reply to message #345928] Fri, 17 June 2016 11:42 Go to previous messageGo to next message
Deleted.

 
Messages:2663
Registered:December 2012
Location: Russian Federation
There are many places in the code where the game accesses incorrect memory, for example in Soldier Control.cpp:
INT8		SOLDIERTYPE::GetUniformType( )
{
	// we determine wether we are currently wearing civilian or military clothes
	for ( UINT8 i = UNIFORM_ENEMY_ADMIN; i <= NUM_UNIFORMS; ++i )
	{
		// both parts have to fit. We cant mix different uniforms and get soldier disguise
		if ( COMPARE_PALETTEREP_ID( this->VestPal, gUniformColors[i].vest ) && COMPARE_PALETTEREP_ID( this->PantsPal, gUniformColors[i].pants ) )
		{
			return i;
		}
	}

	return -1;
}

gUniformColors is defined as
// Array to hold uniform data
UNIFORMCOLORS gUniformColors[NUM_UNIFORMS];

so it starts from 0 to NUM_UNIFORMS - 1



Left this community.

Report message to a moderator

Lieutenant

Re: Horrible Encyclopedia code crashes the game[message #345933 is a reply to message #345930] Fri, 17 June 2016 21:42 Go to previous messageGo to next message
Flugente

 
Messages:3509
Registered:April 2009
Location: Germany
Well, then each of these bugs should be fixed ASAP by whoever caused it - in this case, me not sure


I know now that it could never work between us, as much as we wanted to, it could never be! Not because you're a rabbit, but because you're black.

If you want, you can donate to me. This will not affect how and what I code, and I will not code specific features in return. I will be thankful though.

Report message to a moderator

Captain

Re: Horrible Encyclopedia code crashes the game[message #345937 is a reply to message #345933] Fri, 17 June 2016 22:43 Go to previous messageGo to next message
Deleted.

 
Messages:2663
Registered:December 2012
Location: Russian Federation
Flugente maybe you will also take a look at some lua issues that I found when you have time?

in LuaInitNPCs.cpp
assignment instead of '==' check
//------Fact and Quest------
if ( bQuests = TRUE )
{
lua_register(L, "SetFactTrue", l_SetFactTrue);
lua_register(L, "SetFactFalse", l_SetFactFalse);
}

if ( bQuests = TRUE )
{
lua_register(L, "StartQuest", l_StartQuest);
lua_register(L, "EndQuest", l_EndQuest);
}


index can exceed array upper bound:
// Flugente: I assume '<= 1000' is meant here...
if ( val >= 0 && val <= 1000 ) 
	gLuaGlobal[val].fGlobalLuaBool = set;

gLuaGlobal is defined as:
LUA_GLOBAL	gLuaGlobal[1000];


in lua_tactical.cpp:
for (int sold=0; sold < 256; sold++)
{
	lua_pushinteger( L, sold);
	if (MercPtrs[ sold ] )
	{
		NewLuaObject( L, SOLDIER_CLASS, MercPtrs[ sold ] );
	}
	else
	{
		lua_pushnil( L );
	}
	lua_settable( L, -3);
}

MercPtrs is defined as
SOLDIERTYPE*    MercPtrs[ TOTAL_SOLDIERS ];

where TOTAL_SOLDIERS cannot be more than 254



Left this community.

Report message to a moderator

Lieutenant

Re: Horrible Encyclopedia code crashes the game[message #345950 is a reply to message #345937] Sun, 19 June 2016 00:16 Go to previous messageGo to next message
Flugente

 
Messages:3509
Registered:April 2009
Location: Germany
Well, I've done that, but that's just the tip of the iceberg. The entire lua-function complex is infested with garbage code, one can literally spend hours fixing all that.

By the way, you also have svn access, you don't have to take a detour over me cheeky



I know now that it could never work between us, as much as we wanted to, it could never be! Not because you're a rabbit, but because you're black.

If you want, you can donate to me. This will not affect how and what I code, and I will not code specific features in return. I will be thankful though.

Report message to a moderator

Captain

Re: Horrible Encyclopedia code crashes the game[message #346241 is a reply to message #345950] Fri, 15 July 2016 13:25 Go to previous messageGo to next message
Flugente

 
Messages:3509
Registered:April 2009
Location: Germany
Soooo... any news on a fix for this? Or can I go on and nuke whatever parts of that feature seem suspicious to me? Or shall I move?


I know now that it could never work between us, as much as we wanted to, it could never be! Not because you're a rabbit, but because you're black.

If you want, you can donate to me. This will not affect how and what I code, and I will not code specific features in return. I will be thankful though.

Report message to a moderator

Captain

Re: Horrible Encyclopedia code crashes the game[message #346244 is a reply to message #346241] Fri, 15 July 2016 14:24 Go to previous messageGo to next message
silversurfer

 
Messages:2793
Registered:May 2009
Considering that Jazz didn't bother to fix other features he implemented (DifficultySettings.xml comes to mind...) I would say "Nuke it!". No coder minds fixing a few bugs of other coders once in a while but cleaning up after someone that doesn't care about what he has done is a big much to ask.



Wildfire Maps Mod 6.07 on SVN: https://ja2svn.mooo.com/source/ja2/branches/Wanne/JA2%201.13%20Wildfire%206.06%20-%20Maps%20MOD

Report message to a moderator

Lieutenant
Re: Horrible Encyclopedia code crashes the game[message #346246 is a reply to message #346244] Fri, 15 July 2016 16:20 Go to previous messageGo to next message
wanne (aka RoWa21) is currently offline wanne (aka RoWa21)

 
Messages:1961
Registered:October 2005
Location: Austria
No reply from Jazz yet. Is there a possibility to fix his code or would you like to disable it? If you like to disable it please put a define around the encyclopedia code parts instead of completly removing the code.
Of course a fix would be better ;)

Report message to a moderator

Sergeant Major

Re: Horrible Encyclopedia code crashes the game[message #346248 is a reply to message #346246] Fri, 15 July 2016 19:34 Go to previous messageGo to next message
Gambigobilla

 
Messages:693
Registered:July 2008
Nuking it seems to be a better option. Maybe he becomes offended and won't push any useless code anymore.

Report message to a moderator

First Sergeant
Re: Horrible Encyclopedia code crashes the game[message #346264 is a reply to message #346248] Sat, 16 July 2016 13:45 Go to previous messageGo to next message
Flugente

 
Messages:3509
Registered:April 2009
Location: Germany
I have no idea what that code should achieve, and I'm done wasting my time with his stuff. Hmpf. I'll #ifdef it for now, but... I hate it when the code is cluttered with useless code. If he doesn't fix it now, I doubt he will do so in the future.

You know what? If this isn't fixed in a few months, I'll remove it for good, for now I'll #ifdef it out.



I know now that it could never work between us, as much as we wanted to, it could never be! Not because you're a rabbit, but because you're black.

If you want, you can donate to me. This will not affect how and what I code, and I will not code specific features in return. I will be thankful though.

Report message to a moderator

Captain

Re: Horrible Encyclopedia code crashes the game[message #346265 is a reply to message #346264] Sat, 16 July 2016 15:05 Go to previous messageGo to next message
Flugente

 
Messages:3509
Registered:April 2009
Location: Germany
As of r8273, the encyclopedia feature is now deactivated.


I know now that it could never work between us, as much as we wanted to, it could never be! Not because you're a rabbit, but because you're black.

If you want, you can donate to me. This will not affect how and what I code, and I will not code specific features in return. I will be thankful though.

Report message to a moderator

Captain

Re: Horrible Encyclopedia code crashes the game[message #346267 is a reply to message #346265] Sat, 16 July 2016 18:22 Go to previous messageGo to next message
Gambigobilla

 
Messages:693
Registered:July 2008
I'm wish someone will remove the externalized difficulty, too. Coincidentally it was coded by Jazz too.

Report message to a moderator

First Sergeant
Re: Horrible Encyclopedia code crashes the game[message #346270 is a reply to message #346267] Sun, 17 July 2016 00:11 Go to previous messageGo to next message
silversurfer

 
Messages:2793
Registered:May 2009
Gambigobilla wrote on Sat, 16 July 2016 17:22
I'm wish someone will remove the externalized difficulty, too. Coincidentally it was coded by Jazz too.

I oppose that as it would remove lots of externalized values that were hard coded before. I like to change these (within current difficulty levels of course).



Wildfire Maps Mod 6.07 on SVN: https://ja2svn.mooo.com/source/ja2/branches/Wanne/JA2%201.13%20Wildfire%206.06%20-%20Maps%20MOD

Report message to a moderator

Lieutenant
Re: Horrible Encyclopedia code crashes the game[message #346343 is a reply to message #345928] Sat, 23 July 2016 12:01 Go to previous message
RunAwayScientist is currently offline RunAwayScientist

 
Messages:85
Registered:September 2001
Flugente wrote on Thu, 16 June 2016 22:59

THIS MEANS THAT WE ACCESS WHATEVER IS IN MEMORY NEXT AND INTERPRET THAT AS A STRING!




Wow, okay. This might actually explain some very odd bugs I have gotten related to long game times, which started happening some time after the encyclopedia was introduced. They were primarily GUI related/triggered, or enemy patrol/Queen decision related. I had no idea how to explain why it caused my game to freeze, black screen, or crash, but corrupted memory after a long period of gameplay may be explained by this.


Generally these bugs occured after mid-game, perhaps 80+ days into a campaign on older trunks. If this array was accessed during play and could be held responsible for general memory corruption (aggravated from save game conversions from a previous ver to the next) this might explain these random freezes. If it's not accessed after visiting the Encylopedia once or twice, then nevermind.


I use a heavily modded/custom version of JA2 that includes carryovers from previous trunk versions. I assumed this might somehow be related, but it was definitely coming from the .exe .


Thank you Flugente. Thank you for your dedication to good coding practices.




Flugente wrote on Thu, 16 June 2016 22:59

First, we never check for NULL-pointers. This means we happily crash when we get one.
Second (and much, MUCH worse) is that we never check for the end of an array. pEncyclopediaSubFilterQuestText etc. are of limited size, but we continue looping even if we reach their end.



You're kidding, right? This is like kindergarten coding 101. Especially the second item. How drunk do you have to be to not remember to check for the size of the array in a loop?!?!?!!?!? How does this code even run?!



Report message to a moderator

Corporal 1st Class
Previous Topic: Drassen Airport Freeze
Next Topic: [Urgent!] XML Editor Start up error
Goto Forum:
  


Current Time: Fri Apr 19 22:40:30 GMT+3 2024

Total time taken to generate the page: 0.01981 seconds