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
|
|
Flugente |
![](/images/ranks/captain.png) |
Messages:3507
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
|
|
|
|
|
|
|
Re: Horrible Encyclopedia code crashes the game[message #345937 is a reply to message #345933]
|
Fri, 17 June 2016 22:43 ![Go to previous message Go to previous message](/theme/Bear_Classic_Brown/images/up.png)
|
|
Deleted. |
![](/images/ranks/liutenant.png) |
Messages:2656
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
|
|
|
|
Re: Horrible Encyclopedia code crashes the game[message #345950 is a reply to message #345937]
|
Sun, 19 June 2016 00:16 ![Go to previous message Go to previous message](/theme/Bear_Classic_Brown/images/up.png)
|
|
Flugente |
![](/images/ranks/captain.png) |
Messages:3507
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
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
|
|
|
|
|
|
|
|
Re: Horrible Encyclopedia code crashes the game[message #346264 is a reply to message #346248]
|
Sat, 16 July 2016 13:45 ![Go to previous message Go to previous message](/theme/Bear_Classic_Brown/images/up.png)
|
|
Flugente |
![](/images/ranks/captain.png) |
Messages:3507
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
|
|
|
|
|
|
|
Re: Horrible Encyclopedia code crashes the game[message #346343 is a reply to message #345928]
|
Sat, 23 July 2016 12:01
|
|
RunAwayScientist |
![](http://thepit.ja-galaxy-forum.com/images/avatars/204.jpg) ![](/images/ranks/corporal_1stclass.png) |
Messages:84
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
|
|
|
|
Goto Forum:
Current Time: Thu Feb 13 19:41:28 GMT+2 2025
Total time taken to generate the page: 0.01735 seconds
|