[portej05]

I've attached the preFAST (/analyze) output for 5199.
It's purely for interest. At least some of the warnings are for scenarios that could never occur.

(More info on preFAST)

[attachment deleted by ninja]

[karajorma]

code - 0 error(s), 1254 warning(s)

Sounds about right

[portej05]

But keep in mind that those are analysis warnings.... They may or may not be warnings, and they may or may not be right. So, is a warning a warning if it may not be a warning?

[Flipside]

...

Come back BBC Basic! All is forgiven!

[karajorma]

But keep in mind that those are analysis warnings.... They may or may not be warnings, and they may or may not be right. So, is a warning a warning if it may not be a warning?

I know that a bunch of them are unreachable but it's still a rather high number I suspect.

[portej05]

The one that I've had a look at is:

Code: [Select]

code\autopilot\autopilot.cpp(1061) : warning C6200: Index '399' is out of valid index range '0' to '74' for non-stack buffer 'struct wing * Wings'


Code: [Select]

if (Ships[i].wingnum != -1)
{
for (j = 0; j < MAX_AI_GOALS; j++)
{
ai_goal *aigp = &Wings[i].ai_goals[j];

if ( ((aigp->ship_name != NULL) && !stricmp(aigp->ship_name, goal_name))
&& (aigp->ai_mode == goal) )

If I'm reading this right, if a ship in a slot above 75 is allocated to a wing, the game'll crash - but my understanding of the ship structure is not good atm

EDIT: To clarify: i goes from 0-399, Wings only has 75 elements

[karajorma]

Unless I'm mistaken the code is actually assuming that the Wing index and the ship index are the same number. And they almost certainly aren't.

I haven't looked at the code but I suspect that was meant to read

Code: [Select]

ai_goal *aigp = &Wings[Ships[i].wingnum].ai_goals[j];

EDIT : Look at this if you get the chance too. You may have found the cause.

[portej05]

yeah, was looking at that earlier

Is there anywhere where I can submit random small patches?

e.g.

Code: [Select]

bool IsVisited(int nav)
{
if (nav > MAX_NAVPOINTS && nav < 0)
return 0;

if (Navs[nav].flags & NP_VISITED)
return 1;
return 0;
}
should be

Code: [Select]

bool IsVisited(int nav)
{
if (nav => MAX_NAVPOINTS && nav < 0)
return 0;

if (Navs[nav].flags & NP_VISITED)
return 1;
return 0;
}


[taylor]

Unless I'm mistaken the code is actually assuming that the Wing index and the ship index are the same number. And they almost certainly aren't.

I haven't looked at the code but I suspect that was meant to read

Code: [Select]

ai_goal *aigp = &Wings[Ships[i].wingnum].ai_goals[j];

EDIT : Look at this if you get the chance too. You may have found the cause.

Wow, and that was one of my changes too.   I believe that code was actually a copy&paste from somewhere else though, so it might be a good idea to do a quick search for the original in case it was wrong as well.  It was probably just a copy&paste error however and the original source is likely fine.

[karajorma]

Yep, that's exactly what appears to have happened. If you search for ai_goal *aigp = &Wings you find another example only a few lines down where the limit for i is MAX_WINGS.

[chief1983]

So the source is correct but the copy paste is wrong in the different context, right?

[taylor]

Yeah, the code just needed an edit after the paste, but I missed it.  And unfortunately the test mission for this only included a case which worked perfectly with this broken code.

The code itself was part of some changes to fix AI related Mantis bugs with regards to autopilot, so it should all be good once the change that karajorma pointed out has been made.

[chief1983]

Well I'll have Woolie taking a look at it then, we'll see he thinks.  Or ARSPR.

[portej05]

I'm finding a lot of cases like this:

Code: [Select]

popup.cpp(682)
if ( (Game_mode & GM_NORMAL) || ((Game_mode && GM_MULTIPLAYER) && !(Game_mode & GM_IN_MISSION)) ){
game_stop_time();
}

I'm going to change it to

Code: [Select]

popup.cpp(682)
if ( (Game_mode & GM_NORMAL) || ((Game_mode & GM_MULTIPLAYER) && !(Game_mode & GM_IN_MISSION)) ){
game_stop_time();
}

but I'm not sure what effect this is going to have - does anyone know what this will do?

[Flipside]

I don't code in C++, but that looks odd, in C++ does the '&' function return true for two 'false' as well as two 'true' results, with the '&&' function only returning a True for two positive results?

[portej05]

yep. Whoever was coding this stuff was pretty tired, and these are bug causing problems that are almost impossible to track down

There are also a fair few cases of this kinda thing (ambiguous ordering - what happens is compiler dependent):

Code: [Select]

if ( !Player->flags & PLAYER_FLAGS_MATCH_TARGET ) {

This one has got me baffled:

Code: [Select]

if ( (PF_ALLOW_DEAD_KEYS) && (Game_mode & GM_IN_MISSION) ) {

This one will always return true if sv->type is not zero:

Code: [Select]

if(sv->type && SEXP_VARIABLE_NUMBER)

Problem is, I don't know what side effects changing these will have.

[karajorma]

but I'm not sure what effect this is going to have - does anyone know what this will do?

It will fix it.

Go ahead with the change. It was obviously a mistake and as far as I can see fixing it won't cause any change in behaviour. The only two actual game modes are single and multiplayer. With that in mind look at what the code was doing

Code: [Select]

if ( (Game_mode & GM_NORMAL) || ((true) && !(Game_mode & GM_IN_MISSION)) )
So if the game is in SP mode (GM_NORMAL) or some other mode (GM_MULTIPLAYER is the only other mode) when not in mission, do something. i.e exactly what  fixing it will do.

[Flipside]

Must admit, that's why I always break statements like that down into bite-sized sections, it's so easy to sink into Boolean hell.

[portej05]

Kara: So one of GM_MULTIPLAYER or GM_NORMAL will always be defined?

[portej05]

In the multiplayer section:

Code: [Select]

if(
MULTI_CONNECTED(Net_players[idx]) &&
(Net_player != &Net_players[idx]) &&
(
(Net_players[idx].state != NETPLAYER_STATE_DEBRIEF) ||
(Net_players[idx].state != NETPLAYER_STATE_DEBRIEF_ACCEPT) ||
(Net_players[idx].state != NETPLAYER_STATE_DEBRIEF_REPLAY)
)
  ){

The three != cases will always be true when combined with ||. Should they have been &&?