View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001969 | FSSCP | multiplayer | public | 2009-07-27 21:23 | 2010-03-23 11:54 |
| Reporter | FUBAR-BDHR | Assigned To | Genghis | ||
| Priority | high | Severity | crash | Reproducibility | have not tried |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.6.11 | ||||
| Target Version | 3.6.12 RC1 | Fixed in Version | 3.6.12 RC2 | ||
| Summary | 0001969: Standalone crash - missionparse.cpp line 5430 | ||||
| Description | While 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 Information | 3.6.11 self build from 07/24 trunk. | ||||
| Tags | No tags attached. | ||||
|
2009-07-27 21:23
|
debug1.txt (1,646 bytes)
> fs2_open_3_6_11d.exe!get_mission_info(char * filename=0x0012ee9d, mission * mission_p=0x00ef31a8, bool basic=true) Line 5430 + 0xf bytes C++ fs2_open_3_6_11d.exe!mission_parse_get_multi_mission_info(char * filename=0x0012ee9d) Line 5850 + 0x10 bytes C++ fs2_open_3_6_11d.exe!multi_options_process_packet(unsigned char * data=0x01bb33cc, header * hinfo=0x0012f2f4) Line 653 + 0xc bytes C++ fs2_open_3_6_11d.exe!process_packet_normal(unsigned char * data=0x01bb33cc, header * header_info=0x0012f2f4) Line 810 + 0xd bytes C++ fs2_open_3_6_11d.exe!multi_process_bigdata(unsigned char * data=0x01bb3388, int len=178, net_addr * from_addr=0x01bc035c, int reliable=1) Line 989 + 0xd bytes C++ fs2_open_3_6_11d.exe!multi_process_incoming() Line 1098 + 0x1e bytes C++ fs2_open_3_6_11d.exe!multi_do_frame() Line 1165 C++ fs2_open_3_6_11d.exe!game_do_networking() Line 1187 C++ fs2_open_3_6_11d.exe!game_do_state_common(int state=30, int no_networking=0) Line 6819 C++ fs2_open_3_6_11d.exe!game_do_state(int state=30) Line 6832 + 0xb bytes C++ fs2_open_3_6_11d.exe!gameseq_process_events() Line 405 + 0x14 bytes C++ fs2_open_3_6_11d.exe!game_main(char * cmdline=0x00151f0b) Line 7453 + 0x5 bytes C++ fs2_open_3_6_11d.exe!WinMain(HINSTANCE__ * hInst=0x00400000, HINSTANCE__ * hPrev=0x00000000, char * szCmdLine=0x00151f0b, int nCmdShow=1) Line 7527 + 0x9 bytes C++ fs2_open_3_6_11d.exe!__tmainCRTStartup() Line 263 + 0x2c bytes C fs2_open_3_6_11d.exe!WinMainCRTStartup() Line 182 C kernel32.dll!7c817067() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll] |
|
2009-07-27 21:23
|
|
|
2009-07-27 21:24
|
|
|
|
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. |
|
|
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. |
|
|
Here's your audit of strncat and strncpy, in current trunk r5841: http://fsscp.pastebin.com/f49b917f8 |
|
|
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. |
|
|
We have a local implementation of strcpy_s and strcat_s until it is standardized. See globalincs/safe_strings.* |
|
|
Taylor agrees with Genghis, incidentally. In some ways, safe_strings introduces more problems than it solves. |
|
|
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. |
|
|
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. |
|
|
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? :) |
|
|
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. |
|
|
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 ? |
|
|
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. |
|
|
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. |
|
|
If so though, we do need to address the difference between the ISO and the MS implementation. Is that feasible? |
|
|
@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. |
|
|
Setting this issue to resolved. Any further discussion about string functions should take place in a new ticket. |
|
|
Upon investigation, issue was already found to be fixed. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2009-07-27 21:23 | FUBAR-BDHR | New Issue | |
| 2009-07-27 21:23 | FUBAR-BDHR | File Added: debug1.txt | |
| 2009-07-27 21:23 | FUBAR-BDHR | File Added: multi.log | |
| 2009-07-27 21:24 | FUBAR-BDHR | File Added: fs2_open.rar | |
| 2009-11-18 02:13 | chief1983 | Priority | normal => high |
| 2009-11-18 02:13 | chief1983 | Target Version | => 3.6.12 RC1 |
| 2009-11-22 07:25 | portej05 | Status | new => assigned |
| 2009-11-22 07:25 | portej05 | Assigned To | => portej05 |
| 2009-11-22 07:26 | portej05 | Note Added: 0011315 | |
| 2009-11-22 07:26 | portej05 | Status | assigned => acknowledged |
| 2010-01-11 13:37 | portej05 | Note Added: 0011518 | |
| 2010-01-25 17:34 | chief1983 | Note Added: 0011588 | |
| 2010-01-25 17:59 | chief1983 | Note Edited: 0011588 | |
| 2010-03-03 11:21 | karajorma | Assigned To | portej05 => |
| 2010-03-03 11:21 | karajorma | Status | acknowledged => new |
| 2010-03-17 15:23 | Genghis | Note Added: 0011788 | |
| 2010-03-17 15:30 | chief1983 | Note Added: 0011789 | |
| 2010-03-19 07:02 | Goober5000 | Note Added: 0011795 | |
| 2010-03-19 07:04 | Goober5000 | Note Edited: 0011795 | |
| 2010-03-19 14:22 | chief1983 | Note Added: 0011800 | |
| 2010-03-19 16:22 | taylor | Note Added: 0011801 | |
| 2010-03-19 20:10 | chief1983 | Note Added: 0011802 | |
| 2010-03-19 22:56 | taylor | Note Added: 0011804 | |
| 2010-03-20 00:55 | Genghis | Note Added: 0011808 | |
| 2010-03-20 01:04 | chief1983 | Note Added: 0011809 | |
| 2010-03-20 02:04 | Genghis | Note Added: 0011810 | |
| 2010-03-20 16:14 | chief1983 | Note Added: 0011814 | |
| 2010-03-21 03:48 | portej05 | Note Added: 0011815 | |
| 2010-03-23 11:53 | Genghis | Note Added: 0011825 | |
| 2010-03-23 11:54 | Genghis | Note Added: 0011826 | |
| 2010-03-23 11:54 | Genghis | Status | new => resolved |
| 2010-03-23 11:54 | Genghis | Fixed in Version | => 3.6.12 RC2 |
| 2010-03-23 11:54 | Genghis | Resolution | open => fixed |
| 2010-03-23 11:54 | Genghis | Assigned To | => Genghis |
