[karajorma]

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.

[karajorma]

You have 3 changes like this

Code: [Select]

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

[portej05]

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.

[karajorma]

I'm seeing a few cases of changes like this too.

Code: [Select]

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.

Code: [Select]

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.



And what does this change in Multi_kick.cpp mean?

/* This safety net does not protect the function. */

[portej05]

Hi Kara,

Those changes are there because there is a conflicting definition.

e.g.

Code: [Select]

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.

[karajorma]

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.

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.

[portej05]

The best thing to do in these cases is to change the name of the variable.

Will keep that in mind.

[portej05]

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]

[Echelon9]

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]

[portej05]

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.

Code: [Select]

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.

[Echelon9]

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.

Code: [Select]

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.

[portej05]

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

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.

[portej05]

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]

[Tomo]

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.

[portej05]

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.

[portej05]

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

[chief1983]

Are they already in VC6?  It sounded like someone said they were originally a MS thing?

[portej05]

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.

[portej05]

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

[karajorma]

I've just noticed, you're not building FRED are you?