crashes inside ffasm.cpp when assigning script slots

Has anybody else experienced this? I am getting crashes even when compiling a blank buffer.

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...

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.

Quote:

---

Originally Posted by DarkDragon

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.

Quote:

---

Originally Posted by Saffith

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)

Code:

---

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

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...

Quote:

---

Originally Posted by DarkDragon

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

Code:

---

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

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.

The crash occurs here:

Code:

---

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

Code:

---

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

---

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

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.

@Saffith 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)

Quote:

---

Originally Posted by DarkDragon

The crash occurs here:

Code:

---

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

Code:

---

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.

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...

Code:

---

(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.

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/Z...f5dc5fa9e509e8