Problem oapiSaveScenario causes access violation

[jedidia]

The title is somewhat simplified, the problem somewhat strange. Here's how it goes:

I wrote an autosave function for IMS that would save the state before module integration. When I start building a new vessel, the first time I integrate a module it executes flawlessly. The second time it executes it causes an access violation in some unrelated code a bit later on. The function seems to execute without trouble otherwise. If I reload the scenario that has been saved by it, and integrate, I get the violation immediately, not only after the second time.

First I thought I'm writing out of bounds in my chararrays, but that's not the case. If I comment out oapiWriteScenario and still execute all the rest in the function, there is no problem.

The next thought was that the violation might be caused somewhere in the save state of my vessel, but that's also a negative. If I save the state just before using the scenario editor, I don't get an access violation. If I execute my autosave function after that, here it is.

I have no idea what exactly the problem could be here. It must have something to do with what I do in the function, but commenting out the oapiSaveScenario call makes the problem go away.
However, commenting out all the rest and passing a fixed filename to oapiWriteScenario also makes the problem go away, so it seems the two of us are working hand in hand on this little crash...

Here's the function:

void IMS::AutoSave(bool finalise)
//makes an autosave of the scenario in an appropriate subfolder. Saves go from 1 through 9, 
//then start overwriting themselves. Every 10. save is permanent. 
{
    char flname[200];
    char desc[200];
    string createPath = "Scenarios/IMS/";
    char VesName[50];
    sprintf(VesName, GetName());

    createPath += VesName;
    CreateDirectory(createPath.data(), NULL);
    int TotModNum = modules.size() + 1;            //because CM isn't included in modules

    if (finalise)
    {
        sprintf(flname, "IMS/%s/%s_before_finalisation", VesName, VesName);    //last save before finalisation gets own filename
    }
    else
    {
        int saveIndex = TotModNum % 10;                //autosaves will be incremental from 1 to 9. Every tenth autosave will not get overwritten, while the increments in between will be
        if (saveIndex == 0) 
        {
            saveIndex = TotModNum /10;  
        }
        sprintf(flname, "IMS/%s/%s_%d", VesName, VesName, saveIndex);
    }
    sprintf(desc, "MJD %lf, %d modules installed", (float)(oapiGetSimMJD()), TotModNum);

    oapiSaveScenario(flname, desc);
}

Note: finalise is always false at the moment.

 

[orb]

So this one is causing exception too, although you pass fixed file name "flname" and description "desc" (they are currently in quotes), and only when you comment out the rest of code it's working fine? :hmm:

 

[jedidia]

Aj sorry, forgot remove the quotes.

Anyways, it does bring up an interesting case. No, it doesn't work in this case, not even with the file name being passed directly. If I comment out the whole rest of the function, it works. If I comment out oapiSaveState, it works. Do both, meh.

 

[Xyon]

In my experience of working with Windows filesystem through C++, I've run into the block a few times where strings used for paths, like this:

string createPath = "Scenarios/IMS/";

should actually be escaped backslashes, like:

string createPath = "Scenarios\\IMS\\";

I'm not certain that will make a difference in your case, but it's certainly been an issue in my code in the past. Have you analysed the return from the calls which use path strings like that to ensure proper completion?

 

[jedidia]

Have you analysed the return from the calls which use path strings like that to ensure proper completion?

Well, I've seen the results, which were exactl as expected. Also, not calling CreateDirectory doesn't get rid of the violation, so we shouldn't have a problem there...

 

[Xyon]

My point is also that your call to oapiSaveScenario also includes forward slashes (in flname). Does changing it make a difference?

 

[jedidia]

Theoretically it shouldn't, as the crash also happens if I pass a filename without subdirectories, but I checked just to make sure. Unfortunately, theory was correct this time. Violation still hapens.

 

[orb]

Windows doesn't care whether they are forward slashes of backslashes, (if path doesn't start with "\\?\") both are accepted as directory separator, and CreateDirectory is WinAPI function, but some user written programs might (if they parse paths and don't include forward slash in their algorithm).

What if you temporarily omit only creating directory (with the directory left existing) and leave the rest of code as is? It's the only other place that could somehow affect the oapiSaveScenario in this function:

[s]CreateDirectory(createPath.data(), NULL);[/s]

 

[jedidia]

What if you temporarily omit only creating directory (with the directory left existing) and leave the rest of code as is? It's the only other place that could somehow affect the oapiSaveScenario in this function:

Already tried, doesn't make a difference. I think I'll try to port the function to the shuttlePB as a minimum code sample and see if I can reproduce it there.

 

[Xyon]

Okay, file path thing doesn't matter then.

For debug, I'd run thought with finalize forced TRUE to see what happens down that control path. It should help you pinpoint the line that's causing your error.

 

[csanders]

Could it be some sort of problem with some code opening the file, but not closing it? Either your code or Orbiter's?

 

[dbeachy1]

Are you 100% certain you are not overwriting the end of one of your 200-byte local character arrays? Since oapiSaveScenario works when you pass a string literal filename to oapiSaveScenario, that sounds like something is wrong with the character buffers being passed to it.

Can you put a breakpoint on the oapiSaveScenario line in the debugger and examine both character buffers passed to it? The best way to pin down a bug is to step through the code and examine the variables -- much easier than guessing at the problem.

 

[jedidia]

Could it be some sort of problem with some code opening the file, but not closing it? Either your code or Orbiter's?

In this case it's not my job to close the file, as I'm not opening it. oapiSaveState works well enough otherwise and was used thousands of times by Orbiter users in the scenario editor (and was used by me before in Orbiter Galaxy), so I don't believe there is such a blatant error in it.

Are you 100% certain you are not overwriting the end of one of your 200-byte local character arrays?

Positive. flname holds between 18 and 23 characters in my test cases, depending on length of the vessels name of course, but there's ample space left there. That was pretty much the first thing I checked, as I had some nasty expierieces with too short chararrays. And as I said, the function executes properly, and the result is there. I can even load the scenario saved by it without problems...

I'm already working without the description in my tests, so that's not the cause either. If it would be writing past the end of flname for some strange reason, then the violation should also occur if I don't execute oapiSaveScenario.

Let's see what happens when I run this with shuttlePB...

---------- Post added at 04:58 PM ---------- Previous post was at 04:47 PM ----------

Negative. Violation does not happen when running the function in ShuttlePB. Now things are going to get difficult...

---------- Post added at 05:54 PM ---------- Previous post was at 04:58 PM ----------

I'm starting to doubt my sanity here...

The violation always happens in the same place in the code... but that bit of code isn't supposed to execute then. And by isn't supposed, I don't mean "oops, how did that call get here", I mean it isn't there. Anywhere.
The line I can set the last breakpoint before the violation happens is somewhat down the line (there's a lot still being executed after the AutoSave function has been called), and calls DetachChild. Instead of arriving on the next line, I am thrown to another place in the code, even another class, in a function that is only called from clbkPostStep where accessing a vector seems to call the violation. I am completely dumbfounded. How is it possible that I'm thrown to another place inside my code without a call to it anywhere near??
And it only happens if Autosave has been executed. Should I throw my laptop in the bin?

 

[csanders]

The line I can set the last breakpoint before the violation happens is somewhat down the line (there's a lot still being executed after the AutoSave function has been called), and calls DetachChild. Instead of arriving on the next line, I am thrown to another place in the code, even another class, in a function that is only called from clbkPostStep where accessing a vector seems to call the violation. I am completely dumbfounded. How is it possible that I'm thrown to another place inside my code without a call to it anywhere near??
And it only happens if Autosave has been executed. Should I throw my laptop in the bin?

I would wait for the breakpoint before the line that calls "DetachChild" to get hit, set a breakpoint at the beginning of "clbkPostStep", then "continue debugging." Perhaps something in "DetachChild" causes orbiter to call "clbkPostStep"?

 

[SiameseCat]

I'm starting to doubt my sanity here...

The violation always happens in the same place in the code... but that bit of code isn't supposed to execute then. And by isn't supposed, I don't mean "oops, how did that call get here", I mean it isn't there. Anywhere.
The line I can set the last breakpoint before the violation happens is somewhat down the line (there's a lot still being executed after the AutoSave function has been called), and calls DetachChild. Instead of arriving on the next line, I am thrown to another place in the code, even another class, in a function that is only called from clbkPostStep where accessing a vector seems to call the violation. I am completely dumbfounded. How is it possible that I'm thrown to another place inside my code without a call to it anywhere near??
And it only happens if Autosave has been executed. Should I throw my laptop in the bin?

Have you tried looking at the call stack window when the violation occurs? This might show where the function is being called.

I don't think this has anything to do with the problem, but you probably want to use strcpy instead of:

sprintf(VesName, GetName());

What you have should still work, though.

 

[jedidia]

I would wait for the breakpoint before the line that calls "DetachChild" to get hit, set a breakpoint at the beginning of "clbkPostStep", then "continue debugging." Perhaps something in "DetachChild" causes orbiter to call "clbkPostStep"?

That would be a bit absurd, but I did it just to make sure. clbkPostStep is never called during the process.

Have you tried looking at the call stack window when the violation occurs? This might show where the function is being called.

It isn't called. That's what I'm trying to say. I'm not looking where the function gets called, I'm asking how it is possible that I end up there without it getting called. I did a search for it, it is called exactly once in the entire solution, and that is in clbkPostStep. The code executed before the crash is nowhere near it.

I can only hypothesize that something overwrites the pointer to DetachChild and this happens to coincide with the address of the function the crash then happens in. But I don't know about how classes and their functions are managed in memory, so I don't know if that's even possible...

---------- Post added at 06:57 PM ---------- Previous post was at 06:36 PM ----------

Replaced the char[200] with a stringstream just to make sure. No change. It's almost as if the very path name would cause a bug in SaveScenario...??

---------- Post added at 07:14 PM ---------- Previous post was at 06:57 PM ----------

I don't think this has anything to do with the problem, but you probably want to use strcpy instead of:

sprintf(VesName, GetName());

What you have should still work, though.

It should, shouldn't it? However, it doesn't. This was the freakin' problem!!

using strcpy for the operation, now everything works fine. Now, if someone could explain me why, I might even learn something from this whole affair.

Thanks everyone for the help anyways. I'll just call the guys with the straightjacket back and tell them they needn't come after all...

 

[Radu094]

Hmm.. funny one! indeed sprintf SHOULD produce an equivalent string as strcpy, provided you do not have conversion specificators(ie. "%s")in the name.Also, IIRC, there was a difference in the way multi-byte-characters are treated, but that shoudln't matter either.

Anyway, I guess it's water under the bridge now that you got it working, but if you're willing to investigate this further, I'd put some padding arround the local variables(eg.):

    char flname[200];
    char padding1[100];
    char desc[200];
    char padding2[100];
    string createPath = "Scenarios/IMS/";
    char padding3[100];
    char VesName[50];
    char padding4[100];

maybe even memset them all to 0, then watch them and see which one changes. Somewhere, something has to leak.

Also, try doing a sprintf(VesName,"%s",GetName()); If sprintf is indeed the culprit, this should also crash it. But if it doesn't crash, then you know is a conversion specificator problem in the GetName();

 

[Urwumpe]

How did you name the vessel? Something with "%" inside?

 

[dbeachy1]

To expand on Uwrumpe's point, in general it is not safe to copy a string with sprintf(dest, src) because if 'src' has any formatting characters in it sprintf will expect (and try to parse) arguments that were not sent, which would have unpredictable results since it would be reading uninitialized data. The second argument ('src' above) is the format string, so to safely copy a string using sprintf you would need:

sprintf(VesName, "%s", GetName());

But if all you need to do is copy a string without any characters around it, strcpy is more efficient (and more "correct").

Still, it was a subtle bug -- nobody (including me!) caught it at first.

 

[jedidia]

Sorry guys, wrong diagnosis... turned out the Bug wasn't as 100% reliable as I thought, and it decided to not show at moments that made me conclude all the things I posted here. They weren't correct. In the end I was able to reproduce it even by just calling oapiSaveState with a normal filename.

I finally solved it (this time for real, it looks like) by waiting a frame after autosave before proceeding. The integration process is somewhat involved, creates and deletes attachment points and docking ports etc. It looks like those functions do not like to be called right after oapiSaveState, for whatever exact reason. I now call autosave when the button is pressed, and then wait until poststep to start the integration process, which seems to solve the problem.

How did you name the vessel? Something with "%" inside?

Ha, no, but that's a good thing to keepin mind.