Source Code Project Mantis - FSSCP
View Issue Details
0001969FSSCPmultiplayerpublic2009-07-27 17:232010-03-23 07:54
ReporterFUBAR-BDHR 
Assigned ToGenghis 
PriorityhighSeveritycrashReproducibilityhave not tried
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version3.6.11 
Target Version3.6.12 RC1Fixed in Version3.6.12 RC2 
Summary0001969: Standalone crash - missionparse.cpp line 5430
DescriptionWhile hunting down the elusive null vec3d crashes I ran into this new one.

Run-Time check Failure 0000002 - Stack around the variable 'real_fname' was corrupt

Chat log on it:

[17:03] karajorma: No wonder that one is crashing!
[17:03] captain-custard: ?
[17:04] karajorma: strncpy(real_fname, filename, MAX_FILENAME_LEN-1);
[17:04] karajorma: and then later
[17:04] karajorma: strcat(real_fname, FS_MISSION_FILE_EXT);
[17:04] karajorma: So it copies 31 bytes into a 32 byte array
[17:04] chief1983: should be -(1+len(FS_MISSION_FILE_EXT)) or something shouldn't it
[17:05] karajorma: attempts to knock some bytes off the end
[17:05] karajorma: and then concatenates .fs2 on the end
[17:05] karajorma: of course if the attempt doesn't work
[17:05] karajorma: it ends up trying to write more than 32 bytes into a 32 byte array
[17:05] chief1983: shouldn't those functions be strcat_s and such now though?
[17:06] karajorma: Not sure
[17:06] karajorma: I don't know how to fix the issue
[17:06] FUBAR: My codebase is from the end of la

I'm attaching the logs and a copy of the calls.
Additional Information3.6.11 self build from 07/24 trunk.
TagsNo tags attached.
Attached Filestxt debug1.txt (1,646) 2009-07-27 17:23
http://scp.indiegames.us/mantis/file_download.php?file_id=1306&type=bug
log multi.log (885,784) 2009-07-27 17:23
http://scp.indiegames.us/mantis/file_download.php?file_id=1307&type=bug
rar fs2_open.rar (158,071) 2009-07-27 17:24
http://scp.indiegames.us/mantis/file_download.php?file_id=1308&type=bug

Notes
(0011315)
portej05   
2009-11-22 02:26   
There's a lot of use of strncat and strncpy throughout the engine in an attempt to watch the buffers, but a fair few of the length calculations don't appear to be quite right!
strcpy_s and strcat_s should be used wherever strncat and strncpy are being used to try and prevent buffer overruns.
(0011518)
portej05   
2010-01-11 08:37   
I'll do a sweep of the codebase checking our usage of strn* before 3.6.11/12 is RC'd - this should fix a number of things like this.
(0011588)
chief1983   
2010-01-25 12:34   
(Last edited: 2010-01-25 12:59)
Here's your audit of strncat and strncpy, in current trunk r5841:

http://fsscp.pastebin.com/f49b917f8

(0011788)
Genghis   
2010-03-17 11:23   
strcpy_s and strcat_s are not standardized yet and are explicitly not supported by GLibc, so they might cause problems for Gnu builds if used?

strlcpy is also explicitly not supported by GLibc, the only "safe" one that is supported by all toolchains is strncpy, which requires some extra maintenance to ensure that the result is null-terminated.
(0011789)
chief1983   
2010-03-17 11:30   
We have a local implementation of strcpy_s and strcat_s until it is standardized. See globalincs/safe_strings.*
(0011795)
Goober5000   
2010-03-19 03:02   
(Last edited: 2010-03-19 03:04)
Taylor agrees with Genghis, incidentally. In some ways, safe_strings introduces more problems than it solves.

(0011800)
chief1983   
2010-03-19 10:22   
I didn't say it was a good implementation, just that we have an implementation. Can it be fixed up or what? I'd rather solve the problems than ignore them. It _has_ caught a boatload of buffer overruns already, many of which have since been fixed, but there's still more remaining I believe.
(0011801)
taylor   
2010-03-19 12:22   
It would probably be best to just drop strcpy_s/strcat_s and go with strncpy for the immediate issues, then begin moving away from C strings entirely. That's the only way to really fix the problem.
(0011802)
chief1983   
2010-03-19 16:10   
I'm sorry but no one's bothered to explain what the problem is. People just keep saying it causes problems. I've year to hear why, whereas I've seen first hand that it's solved a lot of issues. Maybe that could get put up on the forum so we can all see what's wrong with it? Weren't we talking about that being the official discussion medium? :)
(0011804)
taylor   
2010-03-19 18:56   
1) The proposed ISO spec for the functions differs from the Microsoft versions of the same functions. The included copies we have are only if the Microsoft versions aren't available and therefore cause differing behavior.

2) It's not a blind replacement for strcpy/strcat. The return values need to be checked and handled to avoid further problems later during execution.

3) It's not implemented properly per the proposed ISO spec.

4) It keeps us on C strings, which is a problem, instead of moving us to std::string which offers far more benefits.

5) It is implemented with variable macros which are not part of the C++ spec and produce warnings in g++. Such a feature is only available through extensions in the compiler and is prone to future issues.


Yes it catches problems, but few of them are actually new. Going with the standard and just as safe (if used properly) strncpy would address the same problems while introducing no new ones (like strcpy_s will). Going with std::string in those problem areas would not only address the problems but also better prepare the code for future upgrading.
(0011808)
Genghis   
2010-03-19 20:55   
So, interesting discussion about standards and STL, but what's the action that will make *this* bug "resolved" ?

I noticed in the code that the offending strcat (now on line 5488) has already been converted to a strcat_s, so the specific case named in this bug report is already fixed.

Should this bug remain open until someone does a sweep of the codebase replacing all instances of str[cat|cpy] with the _s version? Should it remain open until someone switches us over to std::string ?
(0011809)
chief1983   
2010-03-19 21:04   
This bug is fixed - we should probably file a new bug to take care of our usages of str* functions. It seems to me though, that in this specific case the strcpy_s was a better choice than strncpy. I don't follow how strncpy is as good a solution here, when it's seen that trying to stuff too big a string caused some big problems. I suppose we'd have to fix the length argument whether it's one or the other, but it seems that the _s functions are doing a better job catching things than the strn* functions. I agree that std::string would be a better solution though, as has pretty much everyone.
(0011810)
Genghis   
2010-03-19 22:04   
In my opinion, str*_s is better for the short term if we're moving to STL later anyway. It copes with the null-termination issue that strn* doesn't, and if the entire codebase is using one convention then it makes a later global search-and-replace sweep easier.
(0011814)
chief1983   
2010-03-20 12:14   
If so though, we do need to address the difference between the ISO and the MS implementation. Is that feasible?
(0011815)
portej05   
2010-03-20 23:48   
@taylor:
1) The reasons the variadic macros are required is to make the behaviour the _SAME_ across all platforms. The SCP safe_strings functions are used on all platforms.
2) Not true. The SCP implementation will assert in debug mode and CTD in release mode.
3) It doesn't need to be implemented as per the ISO spec - the whole point of safe_strings was to get a library of string functions into the engine which made our string handling a lot safer. We've discovered a number of bugs through it, and then engine should be much more stable thanks to it.
4) Until the SCP shows that it understands the STL, I'd steer well clear of std::string. Look at all the problems we found with memset(class,0) killing off vectors.
5) Variadic macros are part of C99, and are likely to make it into the C++ standard at some point. variadic functions aren't appropriate in this case (you can't overload an already specified function). They are however supported on all the compilers we use, and make our job a lot easier.
(0011825)
Genghis   
2010-03-23 07:53   
Setting this issue to resolved. Any further discussion about string functions should take place in a new ticket.
(0011826)
Genghis   
2010-03-23 07:54   
Upon investigation, issue was already found to be fixed.

Issue History
2009-07-27 17:23FUBAR-BDHRNew Issue
2009-07-27 17:23FUBAR-BDHRFile Added: debug1.txt
2009-07-27 17:23FUBAR-BDHRFile Added: multi.log
2009-07-27 17:24FUBAR-BDHRFile Added: fs2_open.rar
2009-11-17 21:13chief1983Prioritynormal => high
2009-11-17 21:13chief1983Target Version => 3.6.12 RC1
2009-11-22 02:25portej05Statusnew => assigned
2009-11-22 02:25portej05Assigned To => portej05
2009-11-22 02:26portej05Note Added: 0011315
2009-11-22 02:26portej05Statusassigned => acknowledged
2010-01-11 08:37portej05Note Added: 0011518
2010-01-25 12:34chief1983Note Added: 0011588
2010-01-25 12:59chief1983Note Edited: 0011588
2010-03-03 06:21karajormaAssigned Toportej05 =>
2010-03-03 06:21karajormaStatusacknowledged => new
2010-03-17 11:23GenghisNote Added: 0011788
2010-03-17 11:30chief1983Note Added: 0011789
2010-03-19 03:02Goober5000Note Added: 0011795
2010-03-19 03:04Goober5000Note Edited: 0011795
2010-03-19 10:22chief1983Note Added: 0011800
2010-03-19 12:22taylorNote Added: 0011801
2010-03-19 16:10chief1983Note Added: 0011802
2010-03-19 18:56taylorNote Added: 0011804
2010-03-19 20:55GenghisNote Added: 0011808
2010-03-19 21:04chief1983Note Added: 0011809
2010-03-19 22:04GenghisNote Added: 0011810
2010-03-20 12:14chief1983Note Added: 0011814
2010-03-20 23:48portej05Note Added: 0011815
2010-03-23 07:53GenghisNote Added: 0011825
2010-03-23 07:54GenghisNote Added: 0011826
2010-03-23 07:54GenghisStatusnew => resolved
2010-03-23 07:54GenghisFixed in Version => 3.6.12 RC2
2010-03-23 07:54GenghisResolutionopen => fixed
2010-03-23 07:54GenghisAssigned To => Genghis