View Issue Details

IDProjectCategoryView StatusLast Update
0001969FSSCPmultiplayerpublic2010-03-23 11:54
ReporterFUBAR-BDHR Assigned ToGenghis  
PriorityhighSeveritycrashReproducibilityhave not tried
Status resolvedResolutionfixed 
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.

Activities

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]	
debug1.txt (1,646 bytes)   

2009-07-27 21:23

 

multi.log (885,784 bytes)

2009-07-27 21:24

 

fs2_open.rar (158,071 bytes)

portej05

2009-11-22 07:26

reporter   ~0011315

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.

portej05

2010-01-11 13:37

reporter   ~0011518

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.

chief1983

2010-01-25 17:34

administrator   ~0011588

Last edited: 2010-01-25 17:59

Here's your audit of strncat and strncpy, in current trunk r5841:

http://fsscp.pastebin.com/f49b917f8

Genghis

2010-03-17 15:23

developer   ~0011788

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.

chief1983

2010-03-17 15:30

administrator   ~0011789

We have a local implementation of strcpy_s and strcat_s until it is standardized. See globalincs/safe_strings.*

Goober5000

2010-03-19 07:02

administrator   ~0011795

Last edited: 2010-03-19 07:04

Taylor agrees with Genghis, incidentally. In some ways, safe_strings introduces more problems than it solves.

chief1983

2010-03-19 14:22

administrator   ~0011800

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.

taylor

2010-03-19 16:22

administrator   ~0011801

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.

chief1983

2010-03-19 20:10

administrator   ~0011802

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? :)

taylor

2010-03-19 22:56

administrator   ~0011804

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.

Genghis

2010-03-20 00:55

developer   ~0011808

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 ?

chief1983

2010-03-20 01:04

administrator   ~0011809

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.

Genghis

2010-03-20 02:04

developer   ~0011810

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.

chief1983

2010-03-20 16:14

administrator   ~0011814

If so though, we do need to address the difference between the ISO and the MS implementation. Is that feasible?

portej05

2010-03-21 03:48

reporter   ~0011815

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

Genghis

2010-03-23 11:53

developer   ~0011825

Setting this issue to resolved. Any further discussion about string functions should take place in a new ticket.

Genghis

2010-03-23 11:54

developer   ~0011826

Upon investigation, issue was already found to be fixed.

Issue History

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