Hard Light Productions Forums
Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: portej05 on May 08, 2009, 07:03:51 am
-
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 (http://www.microsoft.com/whdc/devtools/tools/PREfast.mspx))
[attachment deleted by ninja]
-
code - 0 error(s), 1254 warning(s)
Sounds about right :rolleyes:
-
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?
-
...
Come back BBC Basic! All is forgiven!
-
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.
-
The one that I've had a look at is:
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'
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
-
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
ai_goal *aigp = &Wings[Ships[i].wingnum].ai_goals[j];
EDIT : Look at this (http://www.hard-light.net/forums/index.php/topic,62680.0.html) if you get the chance too. You may have found the cause.
-
yeah, was looking at that earlier
Is there anywhere where I can submit random small patches?
e.g.
bool IsVisited(int nav)
{
if (nav > MAX_NAVPOINTS && nav < 0)
return 0;
if (Navs[nav].flags & NP_VISITED)
return 1;
return 0;
}
should be
bool IsVisited(int nav)
{
if (nav => MAX_NAVPOINTS && nav < 0)
return 0;
if (Navs[nav].flags & NP_VISITED)
return 1;
return 0;
}
-
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
ai_goal *aigp = &Wings[Ships[i].wingnum].ai_goals[j];
EDIT : Look at this (http://www.hard-light.net/forums/index.php/topic,62680.0.html) if you get the chance too. You may have found the cause.
Wow, and that was one of my changes too. :sigh: 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.
-
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.
-
So the source is correct but the copy paste is wrong in the different context, right?
-
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. :rolleyes:
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.
-
Well I'll have Woolie taking a look at it then, we'll see he thinks. Or ARSPR.
-
I'm finding a lot of cases like this:
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
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?
-
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?
-
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):
if ( !Player->flags & PLAYER_FLAGS_MATCH_TARGET ) {
This one has got me baffled:
if ( (PF_ALLOW_DEAD_KEYS) && (Game_mode & GM_IN_MISSION) ) {
This one will always return true if sv->type is not zero:
if(sv->type && SEXP_VARIABLE_NUMBER)
Problem is, I don't know what side effects changing these will have.
-
but I'm not sure what effect this is going to have - does anyone know what this will do?
It will fix it. :D
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
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.
-
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.
-
Kara: So one of GM_MULTIPLAYER or GM_NORMAL will always be defined?
-
In the multiplayer section:
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 &&?
-
yep. Whoever was coding this stuff was pretty tired, and these are bug causing problems that are almost impossible to track down
That sort of mistake is pretty easy to make. The reverse case is pretty easy too (and is even harder to spot if your programming language supports short-circuit evaluation as in that case it may actually be perfectly valid code even though it's not what you intended to do).
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.
That sort of stuff is one of my biggest C programming pet peeves. The compiler should be screaming at the coder that what he's just done is nonsense.
There are also a fair few cases of this kinda thing (ambiguous ordering - what happens is compiler dependent):
if ( !Player->flags & PLAYER_FLAGS_MATCH_TARGET ) {
Yeah, that's almost certainly meant to have had everything after the NOT in brackets.
This one has got me baffled:
if ( (PF_ALLOW_DEAD_KEYS) && (Game_mode & GM_IN_MISSION) ) {
Almost certainly was meant to read if ( (Popup_flags & PF_ALLOW_DEAD_KEYS) && (Game_mode & GM_IN_MISSION) ) . I have no idea what changing this will do though. I can take a look.
This one will always return true if sv->type is not zero:
if(sv->type && SEXP_VARIABLE_NUMBER)
Definitely an error. Where exactly did you see it? I'll take a look to see if it needs any further fixing.
-
I'm bundling these all together, I'm just about to play through some missions to see if there are any obvious adverse affects. Some of these changes could cause really subtle errors.
EDIT: Most of these changes are not bug related. I'm going to play through a few missions to see if there are any obvious problems. I've attached a diff against 5204.
-
I suspect a bunch of them have always caused bugs. For instance Wanderer reports that variable handling in lua has always been quirky. The if(sv->type && SEXP_VARIABLE_NUMBER) error (and the same one for strings below it) would definitely explain that. It simply wouldn't work for string variables as it would try to atoi() them into a number.
You can go ahead and fix that one too.
-
For the PF_ALLOW_DEAD_KEYS one, That line was the only one in the code that refers to PF_ALLOW_DEAD_KEYS in the first place. I'm wondering if we should simply remove the reference to PF_ALLOW_DEAD_KEYS and only check (Game_mode & GM_IN_MISSION) ) or simply comment out the entire statement.
Not really certain to be honest.
-
See attached (numerous, small changes)
EDIT: Attachment eaten. Out of date.
-
Looks good to me. Not so sure about your fix for Mantis 1780 (http://scp.indiegames.us/mantis/view.php?id=1780) though. Fixing the FRED issue by hobbling output in FS2 as well doesn't sit right with me somehow.
But I'll try to figure out what to do about that (if anything) and post it on the Mantis report instead of here.
-
I thought about that, but reasoned it out that if really long text message boxes are causing problems in FRED, really long text message boxes are probably causing problems in FS2.
-
True.
Anyway I've just noticed that on the MSVC 2008 project files the warning level for debug is set to 3 not 4 for FRED and code. I think it would be a good idea to bump that up a bit.
-
Alrighty, everything we've chatted about today is in the attached diff.
I've started to add some of the SAL annotations to help with preFAST, if you see one that is wrong, please let me know.
I'm pretty sure I got the ifdefs right for SAL (check the bottom of pstypes.h after applying the diff), but if someone can compile with something that is VC++ that'd be great.
Played through a bunch of ST:R campaigns (AWESOME), I haven't noticed anything odd with the gameplay (other than a few NULL vectors) - however, there may be some subtle errors.
Feedback would be great.
EDIT: It's a diff against 5204 Trunk
[attachment deleted by ninja]
-
I took a good long look at this and I'm somewhat confused as to what some of the changes actually do. For instance stuff like this.
-void send_autopilot_msg(char *msg, char *snd)
+void send_autopilot_msg( _In_z_ const char *msg, _In_opt_z_ const char *snd)
-
I just realised you're using VC6, so this will probably cause your compile to fail - will add a condition to check the VC version.
What it does is to provide the preFAST compiler with additional information on the intent of the function wrt to parameter use. In terms of a normal compile, it will do nothing at all - I'll also add a debug switch. By doing this, the noise from the preFAST step can be significantly reduced, and calling errors (both to library and internal code) can be picked up.
I'll try and dig up some good resources on it, but a good start is Michael Howards SAL/preFAST blog entry.
-
I'm just not certain we should add something like that to SVN. I'm actually using 2008 but there are plenty who use VC6 and the Mac and Linux coders are going to have issues with it too.
I'll see about adding the rest of your fixes as soon as I can though.
EDIT : I was unsure of the reasons behind this change though
- output += wsprintf(output, "%08x: ", pStack);
+ output += wsprintf(output, "%08p: ", pStack);
in exceptionhandler.cpp. I'm a little confused as to why you're replacing the output in this way.
-
As far as I can tell, the %x specifier is unable to tell the size of the passed variable, and will always use the type 'int' for whatever is passed. So you'll never see an 8byte pointer (for a 64bit compile). The %p specifier will default to the pointer size - 4 bytes on a 32 bit compile and 8 on a 64bit compile - you could probably remove the %08.
pStack in that case is a pointer of type DWORD*, so it made sense to make that change (and it reduced the warning count)
-
Well I've just committed all the other fixes. I'll remember to test and add this one in a bit.
-
I'm just not certain we should add something like that to SVN. I'm actually using 2008 but there are plenty who use VC6 and the Mac and Linux coders are going to have issues with it too.
They should all be #define'd to nothing under any non-preFAST platform (I need to change the VC version check though, and need to ensure that that includes those builds that aren't /analyze under VC++), so it shouldn't cause a problem, but can understand the hesitation.
If nothing else, it'll mean a few people get to scour the entire code base, and potentially add some in-code documentation for each function (useful!) and hopefully flag dodgy stuff they see along the way.
-
Well I'd like to hear the comments of the other coders but personally I feel that although it reduces the number of false positives it also makes the code much harder to read. Especially for anyone looking at the code for the first time.
-
If it's only used in problematic areas (sounds like that was the intent?) and commented where used it probably wouldn't hurt readability enough to get in the way of its potential benefits. I'm not sure I understand it fully yet though.
-
Well I'd like to hear the comments of the other coders but personally I feel that although it reduces the number of false positives it also makes the code much harder to read. Especially for anyone looking at the code for the first time.
The code is a bit intimidating for anyone for the first time :P
-
Here's one not caught by preFAST (was actually looking for something else)
3ddraw.cpp:
//like g3_draw_poly(), but checks to see if facing. If surface normal is
//NULL, this routine must compute it, which will be slow. It is better to
//pre-compute the normal, and pass it to this function. When the normal
//is passed, this function works like g3_check_normal_facing() plus
//g3_draw_poly().
//returns -1 if not facing, 1 if off screen, 0 if drew
int g3_draw_poly_if_facing(int nv,vertex **pointlist,uint tmap_flags,vec3d *norm,vec3d *pnt)
{
Assert( G3_count == 1 );
if (do_facing_check(norm,pointlist,pnt))
return g3_draw_poly(nv,pointlist,tmap_flags);
else
return 255;
}
255 is not -1. Should be (int)-1;
EDIT: It's never called. Never mind.
EDIT2:
There's another case of this on line 702 of 3ddraw.cpp. I can't find a case of where the return value from g3_draw_poly is checked, but it may be worth correcting this anyway.
-
Kara: Some more patches attached. Most of them are just clarifying cases, but there are one or two in there (such as the change in gropengldraw.cpp) that could affect rendering or gameplay.
Also, around line 1544 of aigoals.cpp there's the line atoi( CTEXT(CDR(CDR(node)))); - shouldn't that be eval_num, given what you told me last night?
[attachment deleted by ninja]
-
Kara: Some more patches attached. Most of them are just clarifying cases, but there are one or two in there (such as the change in gropengldraw.cpp) that could affect rendering or gameplay.
I'll take a look but you might as well get started on getting commit access yourself. :)
Also, around line 1544 of aigoals.cpp there's the line atoi( CTEXT(CDR(CDR(node)))); - shouldn't that be eval_num, given what you told me last night?
Yes, it probably should be. I don't think anyone has ever tried using a variable or a SEXP as a goal priority before (or just accepted it as a mistake when it crashed) but in general we should never assume that any SEXP node contains a number. I'll take a look into it.
-
You have 3 changes like this
- *ptr++ = str[j];
+ (*ptr)++ = str[j];
Which won't compile for me. That might be for the best actually cause neither line is that clear about what it's doing. What it's meant to do is
1) Dereference the pointer
2) Set the value to the contents of that slot of the array
3) Increment the pointer.
Judging from the error message I got on compile it seems like the compiler thought I was trying to
1) Dereference the pointer
2) Set the value to the contents of that slot of the array
3) Increment the value.
Since it didn't like me doing the 3rd step, it refused to compile. Which is definitely better than actually trying to do that.
-
Hi Kara,
Interesting case - it did compile for me. Weird.
I agree with that analysis.
Probably best to split it into two lines.
A quick search across the codebase turns up a few others like that.
-
I'm seeing a few cases of changes like this too.
int cur_wing_block, cur_wing_slot, cur_bank;
int weapon_type_to_add, result;
- int i;
ship_info *sip, *source_sip;
weapon_info *wip;
@@ -4938,7 +4937,7 @@
// clear error stuff
error_flag = false;
- for (i = 0; i < MAX_WING_SLOTS * MAX_SHIP_WEAPONS; i++)
+ for (int i = 0; i < MAX_WING_SLOTS * MAX_SHIP_WEAPONS; i++)
*error_messages[i] = '\0';
// make sure we're not holding anything
Don't make changes like that. There is a reason they're written that way. VC6 considers this to be legal.
for (int i = 0; i < something; i++) {
Do something to i;
}
Do something else to i;
In other words a declaration in a for loop is equivalent to a declaration just before one. It's stupid. It's a PITA. And we have to support it. :rolleyes:
And what does this change in Multi_kick.cpp mean?
/* This safety net does not protect the function. */
-
Hi Kara,
Those changes are there because there is a conflicting definition.
e.g.
int i;
dosomethingwithi;
if ( somestatementistrue )
{
int i;
for ( i = 0; i < 10; i++ )
{ ... }
}
Those changes are to protect someone in the future against referencing the wrong i.
Execution in that function continues in the case that pl is NULL (leaving pl NULL when you hit the switch), and pl will always be referenced in that switch statement, if not through a case, then through the default case.
-
Yeah, I'm aware why you changed it. I'm just saying that changes like the one you made introduce their own set of problems. There's very little to stop someone who is using VC6 from referencing the inner i somewhere outside the for loop. It will work perfectly well for them but as soon as they commit it everyone else will find it's broken. And the change someone will make on discovering that is to declare i outside the loop. Which gets us right back to where we started. :D
The best thing to do in these cases is to change the name of the variable.
As for the multi_kick issue. I see your point now. I'll see what I can do about it.
-
The best thing to do in these cases is to change the name of the variable.
Will keep that in mind.
-
For your enjoyment, I've attached the latest preFAST run, and a (HACKJOB WARNING) tool that allows you to view the warnings by group.
[attachment deleted by Tolwyn]
-
portej05,
I think I've fixed up all the potential overflows in movie.cpp. Could you test this patch through preFAST and in a build trying to play a couple of movies/cutscenes?
[attachment deleted by Tolwyn]
-
Hey,
I've had a look at the patch, you've replaced the calls with strncpy, which is just as dangerous! The size of the first parameter buffer is still unknown.
strncpy( tmp_name, filename, MAX_PATH-1 );
Will try and read/copy MAX_PATH-1 characters from filename into tmp_name.
I'm going to stick in a file with the strcpy_s and strcat_s forms when I finish exams (Wednesday).
Recommend not using the patch.
-
Hey,
I've had a look at the patch, you've replaced the calls with strncpy, which is just as dangerous! The size of the first parameter buffer is still unknown.
strncpy( tmp_name, filename, MAX_PATH-1 );
Will try and read/copy MAX_PATH-1 characters from filename into tmp_name.
Yes, but we know the length of tmp_name because it is declared above to be "char tmp_name[MAX_PATH];" and then zero'd out with "memset( tmp_name, 0, sizeof(tmp_name) );".
I realise that strncpy can still be dangerous, because now the programmer has to keep track of how much space is left in the buffer. Here, AFAIK there would have been no other operations on tmp_name before movie.cpp:148 so we do know how much space is left in the buffer. That is part of the reason why I chose movie.cpp first, it's fairly straight forward.
I'm going to stick in a file with the strcpy_s and strcat_s forms when I finish exams (Wednesday).
Wont work.
Remember, FS2Open is a cross platform code base. You can't just up and change to the *_s formats - they are Windows only.
-
I was actually going to write them in an ifdef block so that they were there for non-windows builds (hence why waiting until after exams!).
The third parameters is the number of characters to copy, not the size of the buffer.
MSDN (http://msdn.microsoft.com/en-us/library/xdsywd25(VS.71).aspx)
Security Note strncpy does not check for sufficient space in strDest; it is therefore a potential cause of buffer overruns. Keep in mind that count limits the number of characters copied; it is not a limit on the size of strDest. See the example below. For more information, see Avoiding Buffer Overruns.
-
I took a brief break from study and coded up the first of the functions - strcpy_s and its test functions.
Your mission (whether or not you choose to accept) is to test this function.
I've included some basic tests, but they do not check every possible case.
Please provide me with some more tests!
Note, SAFESTRINGS_TEST_APP needs to be defined for now.
Please also note that this code is not ready to be used, and is TEST CODE ONLY.
EDIT: strcat_s and tests.
[attachment deleted by Tolwyn]
-
The so-called 'safe' strncpy and memcpy functions are nothing of the sort - either way, you have to keep track of the buffer.
- If you forget how large the destination is, the 'safe' versions still give you a buffer under/overrun because they do not check the actual target, instead they check that the size you said it was meets the requirement to send the data you're specifying.
So the only situations they catch is when you do something like
strncpy_s(dst, 5, "a long string", 5);
- This throws an error as there's no space for the NUL
However, if you do
strncpy_s(dst, 5, "a long string", 4);
- if dst only has space for 3 characters, then strncpy_s succeeds but you still have a buffer overflow.
Finally, and most disturbingly, srtncpy_s still has undefined behaviour if Src and Dest overlap - if it handled that one, I'd accept it as an improvement. As it is though, it's simply a slowdown for no gain.
In other words, they are simply a marketing gimmick from Microsoft - this is why none of the other runtime libraries include them.
You know where the data came from, you know how big your buffer is, so check it yourself. If you're not checking it, then you're being a muppet and the 'secure' versions won't save you anyway.
-
They do help when you're playing with buffers on the stack (through the templated versions), which we do a heck of a lot in the engine.
The only functions getting 'done up' are the strcpy and strcat functions.
It is much easier to have a function that does the check than to actually stick if clauses around every instance of their use in the code.
Where they are not so safe is for heap allocated strings.
-
I'd like to add that strcpy_s, strcat_s, strncpy_s and strncat_s are the subject of ISO/IEC TR 24731, and are thus candidates for standardisation into the standard C library.
US-Cert.Gov (https://buildsecurityin.us-cert.gov/daisy/bsi/articles/knowledge/coding/314-BSI.html)
-
Are they already in VC6? It sounded like someone said they were originally a MS thing?
-
Are they already in VC6? It sounded like someone said they were originally a MS thing?
They're not in anything that isn't the runtime shipped with VC2005 or VC2008, hence why we're having to code them.
If/when the TR is accepted and they appear in other platform runtimes, we can remove them as required.
-
OK, this code is now in trunk (safe_strings.[h/cpp])
Note the licence details at the top - safe_strings_test.cpp must remain with the two other files.
I'll add strn*_s if the need arises.
All new code should use str*_s, old code should remain the same for now.
In IRC last night we agreed not to do anything further on this until next week, the idea being that any compile or project problems should arise before next week (so, get compiling folks!).
Echelon9, myself and Kara will be making changes to use this code from next week, and we'd really appreciate any problem reports whatsoever, including buffer overruns (ERANGE asserts) or parameter problems (EINVAL asserts)
portej05
-
I've just noticed, you're not building FRED are you?
-
I can, and have. But that's more warnings than I can handle in one go!
-
HEAD now fixed. (5387)