PDA

View Full Version : crashes inside ffasm.cpp when assigning script slots



DarkDragon
01-09-2017, 01:53 PM
Has anybody else experienced this? I am getting crashes even when compiling a blank buffer.

DarkDragon
01-09-2017, 03:38 PM
Ok, I investigated and there are two problems:

1. Someone at some point increased the number of global scripts to 4 from 3, but didn't allocate space for the extra script in ZQuest; this was easily fixed.
2. The default quest (qst.dat) is corrupt (!!!). It thinks that it contains 5 different global scripts: the first is ~Init (fine), but it also thinks that it contains "slot2_icecombos" and "GhostZHActiveScript" as well as a few more global scripts with corrupt names. It thinks these scripts are assigned to global slots 4+ which of course don't exist, and this is causing the crash.
Gleeok, Saffith do you know what is going on? The qst.dat is one I pulled from the 2.50.2 Windows zip file. I checked the quest saving code and I do not see why it would have created this corruption; is it possible qst.dat was saved using some experimental build of ZQ with extra global slots (the old master, maybe) and accidentally included in 2.50.2?

My main concern is that any new quest created in 2.50.2 and saved there will propagate the corruption...

Saffith
01-09-2017, 04:05 PM
I don't know how those extra scripts got there exactly, but they were removed for 2.50.3. I think those were entirely from qst.dat and weren't actually saved with the individual quests, since they're written in a for(int i=0; i<NUMSCRIPTGLOBAL; i++) loop. Loading an older quest in the current build, I don't see them.

I wouldn't expect them to cause a crash, though, since global scripts are loaded into the array in a similarly limited loop.

ZoriaRPG
01-09-2017, 04:33 PM
Has anybody else experienced this? I am getting crashes even when compiling a blank buffer.

I have had no such crashes with any of the builds that I've done with the 2.54 stuff. Parser changes could certainly do that, though, in my experience.

DarkDragon
01-09-2017, 04:45 PM
I don't know how those extra scripts got there exactly, but they were removed for 2.50.3. I think those were entirely from qst.dat and weren't actually saved with the individual quests, since they're written in a for(int i=0; i<NUMSCRIPTGLOBAL; i++) loop. Loading an older quest in the current build, I don't see them.

I wouldn't expect them to cause a crash, though, since global scripts are loaded into the array in a similarly limited loop.

The writing code looks like: (zq_class.cpp starting at around line 10000)



word numglobalbindings=0;

for(std::map<int, pair<string, string> >::iterator it = globalmap.begin(); it != globalmap.end(); it++)
{
if(it->second.second != "")
{
numglobalbindings++;
}
}

if(!p_iputw(numglobalbindings, f))
{
new_return(2007);
}

for(std::map<int, pair<string, string> >::iterator it = globalmap.begin(); it != globalmap.end(); it++)
{
if(it->second.second != "")
{
if(!p_iputw(it->first,f))
{
new_return(2008);
}

if(!p_iputl((long)it->second.second.size(), f))
{
new_return(2009);
}

if(!pfwrite((void *)it->second.second.c_str(), (long)it->second.second.size(),f))
{
new_return(2010);
}
}
}


and the quest load code


word numglobalbindings;
p_igetw(&numglobalbindings, f, true);

for(int i=0; i<numglobalbindings; i++)
{
word id;
p_igetw(&id, f, true);
p_igetl(&bufsize, f, true);
buf = new char[bufsize+1];
pfread(buf, bufsize, f, true);
buf[bufsize]=0;

//nothing needed here
if(keepdata)
{
//Disable old '~Continue's, they'd wreak havoc. Bit messy, apologies ~Joe
if(strcmp(buf,"~Continue") == 0)
{
globalmap[id].second = "";

if(globalscripts[GLOBAL_SCRIPT_CONTINUE] != NULL)
globalscripts[GLOBAL_SCRIPT_CONTINUE][0].command = 0xFFFF;
}
else
{
globalmap[id].second = buf;
}
}

delete[] buf;
}


The crash occurs if id > 4, since the slot assignment code later on assumes that no script will try to be assigned to global slots > 3.

Somehow qst.dat has corrupt bindings in it, and I don't see anything that will stop these corrupt bindings from being saved out again in new quests.

The fix is not too difficult, and amounts to ignoring any global bindings with id > NUMSCRIPTGLOBAL at quest load time. But I'd like to know how the corruption got into qst.dat in the first place, to make sure it's not an ongoing bug...

ZoriaRPG
01-09-2017, 05:18 PM
The writing code looks like: (zq_class.cpp starting at around line 10000)



word numglobalbindings=0;

for(std::map<int, pair<string, string> >::iterator it = globalmap.begin(); it != globalmap.end(); it++)
{
if(it->second.second != "")
{
numglobalbindings++;
}
}

if(!p_iputw(numglobalbindings, f))
{
new_return(2007);
}

for(std::map<int, pair<string, string> >::iterator it = globalmap.begin(); it != globalmap.end(); it++)
{
if(it->second.second != "")
{
if(!p_iputw(it->first,f))
{
new_return(2008);
}

if(!p_iputl((long)it->second.second.size(), f))
{
new_return(2009);
}

if(!pfwrite((void *)it->second.second.c_str(), (long)it->second.second.size(),f))
{
new_return(2010);
}
}
}


and the quest load code


word numglobalbindings;
p_igetw(&numglobalbindings, f, true);

for(int i=0; i<numglobalbindings; i++)
{
word id;
p_igetw(&id, f, true);
p_igetl(&bufsize, f, true);
buf = new char[bufsize+1];
pfread(buf, bufsize, f, true);
buf[bufsize]=0;

//nothing needed here
if(keepdata)
{
//Disable old '~Continue's, they'd wreak havoc. Bit messy, apologies ~Joe
if(strcmp(buf,"~Continue") == 0)
{
globalmap[id].second = "";

if(globalscripts[GLOBAL_SCRIPT_CONTINUE] != NULL)
globalscripts[GLOBAL_SCRIPT_CONTINUE][0].command = 0xFFFF;
}
else
{
globalmap[id].second = buf;
}
}

delete[] buf;
}


The crash occurs if id > 4, since the slot assignment code later on assumes that no script will try to be assigned to global slots > 3.

Somehow qst.dat has corrupt bindings in it, and I don't see anything that will stop these corrupt bindings from being saved out again in new quests.

The fix is not too difficult, and amounts to ignoring any global bindings with id > NUMSCRIPTGLOBAL at quest load time. But I'd like to know how the corruption got into qst.dat in the first place, to make sure it's not an ongoing bug...

One thing that I can say with certainty, is that this has been in qst.dat since at least 2.50.1, if not 2.50.0RCs. It has never causes crashes on any of the released ZC builds, nor on 2.50.3RC1, nor 2.54. That doesn't mean we shouldn't fix it, and by this, I mean fix improper values called in the loops, but rather, that we also need to know why it was crashing whatever you just built.

It doesn't crash the builds that I made from your tree.

DarkDragon
01-09-2017, 05:24 PM
The crash occurs here:



for(std::map<int, pair<string,string> >::iterator it = globalmap.begin(); it != globalmap.end(); it++)
{
if(it->second.second != "")
{
tempfile = fopen("tmp","w");

if(!tempfile)
{
jwin_alert("Error","Unable to create a temporary file in current directory!",NULL,NULL,"O&K",NULL,'k',0,lfont);
return D_O_K;
}

if(output)
{
al_trace("\n");
al_trace("%s",it->second.second.c_str());
al_trace("\n");
}

for(vector<Opcode *>::iterator line = scripts[it->second.second].begin(); line != scripts[it->second.second].end(); line++)
{
string theline = (*line)->printLine();
fwrite(theline.c_str(), sizeof(char), theline.size(),tempfile);

if(output)
{
al_trace("%s",theline.c_str());
}
}

fclose(tempfile);
parse_script_file(&globalscripts[it->first],"tmp",false);
}
else if(globalscripts[it->first])
{
delete[] globalscripts[it->first];
globalscripts[it->first] = new ffscript[1];
globalscripts[it->first][0].command = 0xFFFF;
}
}


What happens if it->first > 3? The following, none of which is good:
- delete[] gets called on a random memory address that was not new[]ed'
- the results of new ffscript[1]; get written into a random memory address.

Your operating system is not required to kill your program when you write to a bad memory address; it can simply silently corrupt your data instead. If your program is not crashing, that is what's happening.

For example, since the global script variables are allocated like



ffscript *globalscripts[NUMSCRIPTGLOBAL];
ffscript *linkscripts[NUMSCRIPTLINK];


it's likely that on your OS, the code is silently corrupting the linkscripts, instead of crashing.

Saffith
01-09-2017, 05:25 PM
Okay, I'm losing track of what's what here...
The actual script data is in the array globalscripts. globalmap is a std::map<int, pair<string, string> >, which maps script numbers to their names and... It looks like first is just used to display "Slot #: " and such? That seems unnecessary. But yeah, at least the names are saved with the quest.
I'm not seeing why id>3 would be a problem, though. It looks like that's only used for globalmap, and that should be harmless. If it were accessing globalscripts[id], that would be a problem, but I don't see that anywhere.

DarkDragon
01-09-2017, 05:29 PM
@Saffith (http://www.armageddongames.net/member.php?u=38582) Yes, the corruption is in the slot assignments, not in the ZASM buffers themselves.

So the crash does not occur when you first load the quest, but rather, the next time you recompile the scripts. ZQuest does indeed write to globalscripts[id] at that time.

(Also, the title of this thread is unintentionally confusing; the crash inside ffasm.cpp was due to a different bug, which I've already pushed a fix for. The crash I'm describing here occurs in zquest.cpp)

ZoriaRPG
01-09-2017, 05:34 PM
The crash occurs here:



for(std::map<int, pair<string,string> >::iterator it = globalmap.begin(); it != globalmap.end(); it++)
{
if(it->second.second != "")
{
tempfile = fopen("tmp","w");

if(!tempfile)
{
jwin_alert("Error","Unable to create a temporary file in current directory!",NULL,NULL,"O&K",NULL,'k',0,lfont);
return D_O_K;
}

if(output)
{
al_trace("\n");
al_trace("%s",it->second.second.c_str());
al_trace("\n");
}

for(vector<Opcode *>::iterator line = scripts[it->second.second].begin(); line != scripts[it->second.second].end(); line++)
{
string theline = (*line)->printLine();
fwrite(theline.c_str(), sizeof(char), theline.size(),tempfile);

if(output)
{
al_trace("%s",theline.c_str());
}
}

fclose(tempfile);
parse_script_file(&globalscripts[it->first],"tmp",false);
}
else if(globalscripts[it->first])
{
delete[] globalscripts[it->first];
globalscripts[it->first] = new ffscript[1];
globalscripts[it->first][0].command = 0xFFFF;
}
}


What happens if it->first > 3? The following, none of which is good:
- delete[] gets called on a random memory address that was not new[]ed'
- the results of new ffscript[1]; get written into a random memory address.

Your operating system is not required to kill your program when you write to a bad memory address; it can simply silently corrupt your data instead. If your program is not crashing, that is what's happening.

For example, since the global script variables are allocated like



ffscript *globalscripts[NUMSCRIPTGLOBAL];
ffscript *linkscripts[NUMSCRIPTLINK];


it's likely that on your OS, the code is silently corrupting the linkscripts, instead of crashing.


Gorgeous. That could in fact, explain some things that have occurred that I've never been able to explain. Absolutely gorgeous.

Saffith
01-09-2017, 05:45 PM
Yeah, I see it now. Sorry, kind of slow today.

I don't know when that originally happened, but GhostZHActiveScript didn't exist until August 2011.

qst.dat from build 1424 seems to be okay; build 1440's is bad. It's not the same, though...

(gdb) print globalmap
$4 = std::map with 6 elements = {[0] = {first = "Slot 1: ~Init",
second = "~Init"}, [1] = {first = "", second = ""}, [2] = {first = "",
second = ""}, [3] = {first = "", second = ""}, [51744] = {first = "",
second = <incomplete sequence \304>}, [52048] = {first = "",
second = "H\200\212\016\240y\226\016nit"}}

Build 1649 is when GhostZHActiveScript and slot2_icecombos first appeared.

DarkDragon
01-10-2017, 02:50 AM
Thanks. It's very strange... I suppose it must be data leaking from one quest into the next one that's opened? But I don't see how, looking at the quest loading and saving code.

In any case this commit is the dead-stupid fix for now: https://github.com/ArmageddonGames/ZeldaClassic/commit/db1d2e16955b27a0e0416b02d1f5dc5fa9e509e8