String Safety

[Baker]

My worst fear in C is the string handling (Wikipedia: Buffer overflow). I understand strings in C fine, but I have to stop and think with some of the functions and their exceptions (strncpy doesn't null terminate a string doing an exact length copy, for example).

A couple of years ago, I made a simple oversight that resulted in a bizarre problem unrelated to any change I made. I have always followed a careful practice of frequent version updates and once I spotted the very stupid mistake I made, I was thankful because otherwise I don't know that I would have been able to spot the mistake since it was totally unrelated to the perceived issue.

I have since paid close attention to changes in DarkPlaces, ezQuake and sometime others to their string handling methods.

snprintf

I've entirely replaced sprintf and vsprintf with snprintf and vsnprintf (yes I know about the dp versions) modelled after Fruitz of Dojo (the Mac Quake port).

Old: sprintf(filestring,"%s/*", host_parms.basedir);
New: snprintf(filestring, sizeof(filestring),"%s/*", host_parms.basedir);

This was rather easy to globally replace using a POSIX Search/Replace in TextPad5 -- and important to me to NOT do it manually -- a single typo on my part could have the same effect as what I seek to fix.



POSIX replace

Code: Select all

Find: sprintf (\([]&[.a-zA-Z0-9_]+\),
Replace With: snprintf(\1, sizeof(\1),

strcpy

I hate strcpy. It disturbs me.

I plan to replace it as such:

Old: strcpy (scr_centerstring, str);
New: strncpy (scr_centerstring, str, sizeof(scr_centerstring)-1);

The description of strncpy is "No null-character is implicitly appended to the end of destination, so destination will only be null-terminated if the length of the C string in source is less than num."

So this looks safe to me.

strcat

DarkPlaces uses strlcat ... but I haven't quite walked through it in my head and haven't figured out how I can "robotically" (search and replace) replace it yet.

[Spike]

sizeof(char *) == 4.

strncpy does not null terminate, correct. malloc does not fill with nulls either.
strlcpy is your friend. :)
If you are sure that the destination is null terminated (ie: bss variables) then subtracting 1 from the max length will ensure that it can never be over length.
Alternatively, memcpy is faster.

strncat... seriously, never use this function, at least not without signing the waiver first. The length argument is that of the source, not of the dest.

_snprintf - WARNING: _snprintf does _not_ null terminate!
linux/bsd snprintf does, but the windows (msvc/mingw) version with the leading underscore does not! Also the return values are different, but whatever.

strcat/strcpy are not unreasonable, assuming you already know the input length of the string and thus that the output is adequate.



The GNU idealism is that you shouldn't ever need to use stuff like strncpy anyway, as you would presumably have ensured that the input was not too long and gracefully handled the error (eg: Host_EndGame... but without the longjmp).
What is certainly true is that simply ignoring that the source was too long and truncating it without really caring can result in additional, more obscure bugs.
In which case, mallocing a new buffer and memcpying the string into it is the ideal way. string length limits? not for me, matey!

Your mileage may vary.

[Baker]

_snprintf - WARNING: _snprintf does _not_ null terminate!

Sigh ...

_snprintf(filestring, sizeof(filestring)-1,"%s/*",

Now I get the "doesn't comply to ISO C standards" or whatever about _snprintf.

What is certainly true is that simply ignoring that the source was too long and truncating it without really caring can result in additional, more obscure bugs.

For the most part, I manually look over the network code (including demo stuffs) and pr_ stuff and system stuff and things involving the client and server (sv_main.c, cl_parse.c, etc.).

I do understand that in some cases unterminated strings are possible and intended.

In which case, mallocing a new buffer and memcpying the string into it is the ideal way. string length limits? not for me, matey!

Hmmmm. I see. Never would have thought of that.

[Spike]

Also be sure to check all your console commands and cvars, thanks to stuffcmd (record ../../boot.ini anyone?).
You're also checking all the writebyte indexes?
Oh, and the bsp loading code is particuarly lazy.
Yeah, kinda trolling, but the point still stands. :P

[Baker]

Also be sure to check all your console commands and cvars, thanks to stuffcmd (record ../../boot.ini anyone?).
You're also checking all the writebyte indexes?
Oh, and the bsp loading code is particuarly lazy.
Yeah, kinda trolling, but the point still stands.

0. "writebyte indexes?" Not sure what type of vulnerability you mean ...

Code: Select all

void MSG_WriteByte (sizebuf_t *sb, int c)
{
	byte    *buf;

#ifdef PARANOID
	if (c < 0 || c > 255)
		Sys_Error ("MSG_WriteByte: range error");
#endif

	buf = SZ_GetSpace (sb, 1);
	buf[0] = c;
}

1. [Q1Source] Cvars -- don't they have dynamically allocated names and string values?

Code: Select all

typedef struct cvar_s
{
	char	*name;
	char	*string;
	qboolean archive;		// set to true to cause it to be saved to vars.rc
	qboolean server;		// notifies players when changed
	float	value;
	struct cvar_s *next;
} cvar_t;

2a. [Q1Source] demo record won't allow such a thing as "record ../../.." I think EZQuake is the only engine that allows recording demos outside the Quake folder and I think they use Com_ForceExtension on it.

Code: Select all

void CL_Record_f (void)
{
	int		c;
	char	name[MAX_OSPATH];
	int		track;
.
.
.

	if (strstr(Cmd_Argv(1), ".."))
	{
		Con_Printf ("Relative pathnames are not allowed.\n");
		return;
	}

2b. Modern engines that allow writing outside of the Quake folder are somewhat proofed against this if they are using COM_ForceExtension() on the filenames. It will change/append the extension name to .dem, for example.

[mh]

Nothing in the engine should write outside of the Quake folder, period. The person who downloads your engine in investing an awful lot of trust in you and you better repay that trust with responsibility.

Reading outside of it is however OK (how else are you gonna load OS libraries? )

Regarding malloc, the best approach is to wrap it. Write your own Q_Malloc (or whatever) that calls malloc, checks the return value for you (also with an assert so that your debugger will catch it) and memsets 0 as well. Then get yourself into the habit of using your wrapper instead of malloc. Also wrap free in a #define that NULLs the pointer you're freeing, while you're at it. Having to write code to check the return values of every malloc yourself is not a productive use of your time, and is too mistake-prone.

MSVC memset operates byte-by-byte according to the source code, so it's not really that fast. Likewise memcpy. You can do better yourself.

[Baker]

Strlcat implementation from DarkPlaces ...

strlcat (name, ".wav", sizeof (name));

So ...

Old ...
1. strcat (s1, s2)
2. strlcat (s1, s2, sizeof(s1))

And I guess if I'm going to be using LH's method for strcat, I might as well use his for strcpy solution as well

Old: strcpy(gl_driver, name);
New: strlcpy(gl_driver, name, sizeof(gl_driver));

strlcat strlcpy

What would be REALLY fun to do some time is to do a #define to catch buffer overruns and Con_Printf them and actually see if these ever occur during a run of the mill day playing mods or on a server.

Code: Select all

//========================================================
// strlcat and strlcpy, from OpenBSD

/*
 * Copyright (c) 1998 Todd C. Miller <Todd.Miller@courtesan.com>
 *
 * Permission to use, copy, modify, and distribute this software for any
 * purpose with or without fee is hereby granted, provided that the above
 * copyright notice and this permission notice appear in all copies.
 *
 * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
 * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
 * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
 * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
 * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 */

/*	$OpenBSD: strlcat.c,v 1.11 2003/06/17 21:56:24 millert Exp $	*/
/*	$OpenBSD: strlcpy.c,v 1.8 2003/06/17 21:56:24 millert Exp $	*/


#ifndef HAVE_STRLCAT
size_t
strlcat(char *dst, const char *src, size_t siz)
{
	register char *d = dst;
	register const char *s = src;
	register size_t n = siz;
	size_t dlen;

	/* Find the end of dst and adjust bytes left but don't go past end */
	while (n-- != 0 && *d != '\0')
		d++;
	dlen = d - dst;
	n = siz - dlen;

	if (n == 0)
		return(dlen + strlen(s));
	while (*s != '\0') {
		if (n != 1) {
			*d++ = *s;
			n--;
		}
		s++;
	}
	*d = '\0';

	return(dlen + (s - src));	/* count does not include NUL */
}
#endif  // #ifndef HAVE_STRLCAT


#ifndef HAVE_STRLCPY
size_t
strlcpy(char *dst, const char *src, size_t siz)
{
	register char *d = dst;
	register const char *s = src;
	register size_t n = siz;

	/* Copy as many bytes as will fit */
	if (n != 0 && --n != 0) {
		do {
			if ((*d++ = *s++) == 0)
				break;
		} while (--n != 0);
	}

	/* Not enough room in dst, add NUL and traverse rest of src */
	if (n == 0) {
		if (siz != 0)
			*d = '\0';		/* NUL-terminate dst */
		while (*s++)
			;
	}

	return(s - src - 1);	/* count does not include NUL */
}

#endif  // #ifndef HAVE_STRLCPY

@MH regarding malloc ... a couple of years ago I changed all of them to Q_mallocs after noticing the change in other engines.

[mankrip]

Yes, there are also several (possible) other sources of buffer overflows in the engine.

I fixed one of them in my engine a while back, it was an array of keys which size didn't match the amount of keys used by the engine.

In KallistiOS (the Dreamcast's main homebrew OS/API), feof (FILE *) doesn't actually work, so to read savegames from the VMU I had to resort to adding a null terminator at the end of the whole file when saving it, so I could make the engine to stop reading the file when that null terminator comes up.

Things like these makes me see the beauty of Java. There should exist better string implementations available in C, but the whole memory management in Java is amazing; even when the compiler doesn't detect a buffer overflow in the program, the virtual machine will, and it will point exactly where's the code that's causing it.

[Baker]

Nothing in the engine should write outside of the Quake folder, period. The person who downloads your engine in investing an awful lot of trust in you and you better repay that trust with responsibility.

I'm not very Linuxy but don't some of the distros by default disallow writing in the Quake folder by default because there isn't a write priviledge so screenshots have to go into whatever the users folder is.

Quake 3 and most games on Windows write screenshots in My Documents.

Then again where is Quakeworld map/model/sound download writing files on these Linux distros? Maybe the same place?

[Likewise, you can tell with Vista and Windows 7 really would prefer you not write application data in the Program Files folder but in the User's appdata folder or whatever. Hence some older apps are broke with UAC on like -- I'm thinking --- the Worldcraft compiling process which writes to Program Files.]

[Sajt]

In my newest project I use no strcpy, no strcat, no sprintf.

You can do what Spike said and malloc every string, I basically do that but slightly "optimized" to avoid allocs on small strings.

I have a struct called "sdstring_t" (which lamely stands for static+dynamic string). You use it like so:

Code: Select all

sdstring_t sdstring;
unsigned char static_buffer[256];

SDString_Init(&sdstring, sizeof(static_buffer), static_buffer);
SDString_Copyf(&sdstring, "Blah %s", "blah");
SDString_Concatf(&sdstring, "ahfdsfj %d asdg", 12345);
printf("It's %s!\n", sdstring.string);
SDString_Free(&sdstring);

It's somewhat cumbersome to use but I'm obsessive about limiting huge amounts of small allocs. Note if you're using C++ this could become very nice and elegant, using a template for the static size. (Also, I did a bit of research on the "alloca" function, which could possibly have made this a bit "neater" to use if combined with macros, but decided it was a bad idea.)

The functions:

Code: Select all

SDString_Init // initialize to blank string (since it can't be NULL).
SDString_Free // free dynamic alloc if it was made.
SDString_Clear // set to blank string, same as SDString_Copy(&sdstring, "");

// These functions will alloc only if the new string doesn't fit in
// the static buffer. If this alloc fails they'll return NULL and leave
// the string alone, otherwise they return sdstring->string
SDString_Copy(sdstring_t *sdstring, const char *src);
SDString_Copyv(sdstring_t *sdstring, const char *format, va_list argptr);
SDString_Copyf(sdstring_t *sdstring, const char *format, ...);
SDString_Concat(sdstring_t *sdstring, const char *src);
SDString_Concatv(sdstring_t *sdstring, const char *format, va_list argptr);
SDString_Concatf(sdstring_t *sdstring, const char *format, ...);

The strings never call any str* standard function except for strlen, which is only used to determine the length of the input string on in non-printf-formatted functions. After that only memcpy is used.

I reimplemented printf-formatting myself for the varargs functions. This is done using a function called "mvformat":

Code: Select all

size_t mvformat(char *buffer, const char *format, va_list argptr);

If you pass NULL as the buffer, it will calculate the length of the string and return it. So the method is to call it once to determine length, alloc if necessary, then call it again to fill the string.

If you want the source to the SDString functions I can post them. But mvformat only has a few formatting types implemented right now.

[Lardarse]

Nothing in the engine should write outside of the Quake folder, period. The person who downloads your engine in investing an awful lot of trust in you and you better repay that trust with responsibility.

I'm not very Linuxy but don't some of the distros by default disallow writing in the Quake folder by default because there isn't a write priviledge so screenshots have to go into whatever the users folder is.

[Likewise, you can tell with Vista and Windows 7 really would prefer you not write application data in the Program Files folder but in the User's appdata folder or whatever. Hence some older apps are broke with UAC on like -- I'm thinking --- the Worldcraft compiling process which writes to Program Files.]

The convention to apply on *nix is to write to a folder called .appname in the user's home folder. A similar convention applies for modern Windows, but I don't know exactly what it is.

These are the only exceptions to the rule of thumb that mh stated, but I would still prefer that the option to go completely single folder be available, at the very least as a compile-time option.

[revelator]

vista and win7 store those in appdata ibelieve atleast on 7 its appdata.

[mh]

Since Windows 2000 (I don't know enough about NT4 to say) the convention has been to store in the user profile. It's better to use the API calls to retrieve the correct folder location, as (1) foreign language versions of Windows may call the folder by a different name, (2) a network administrator may have redirected it to a network share, or (3) the user may have a roaming profile. (Trust me, I did this stuff for a living for 12 years! )

Stuff like config settings should go into %appdata%, the reasoning being that the user shouldn't mess with them directly but instead use whatever interface is available in the program for setting them. Note that this isn't security, and nor is it meant to be. In general with Windows if you have to access a hidden folder or do any jiggerypokery it's intended as a signal that "you are not supposed to be here, there is an easier and safer way of doing what you want to do, maybe you should go read the documentation and do it that way instead".

Even better, store them in the registry. That way you don't need to worry about crap like roaming profiles or foreign languages, you get a nice fast hierarchical database that's optimised for speedy retrieval of settings, and you don't need to write multiple text file parsers for multiple different text file formats. ("Registry bloat slows down your computer" is bullshit propagated by dubious vendors of registry "cleaning tools", and by Weenix Loonies. Don't believe it, it's a lie.)

Stuff like game saves and screenshots should go in My Documents, in a program-specific subfolder. These are things that the user may wish to access, so make them accessible. Again, use the API to get the path for the same reasons as mentioned above.

Code: Select all

SHGetFolderPath (NULL, CSIDL_PERSONAL, NULL, 0, szPath);

It's not that difficult.

(There are some major enterprisey applications, not mentioning names but from the likes of *cough* IBM *cough*, *cough* Oracle *cough* and *cough* Sun *cough* that do not respect this convention and go ahead and store user-specific settings in the program folder anyway.)

Anyway, all of the above would represent a massive change to the way Quake works at several basic levels, and is probably a step too far. But if it was being designed today this is the way it would be designed.

[Sajt]

It keeps changing. It's either AppData, or My Documents, or My Games, or My Documents/My Games, or Local Settings/AppData, ...

I think you are supposed to put "hidden" cache-like stuff (the same sort of thing as GLQuake's ms2 files, for example), in AppData, and put the user-config stuff (screenshots, saves, config) in My Games.

[mh]

It keeps changing. It's either AppData, or My Documents, or My Games, or My Documents/My Games, or Local Settings/AppData, ...

I think you are supposed to put "hidden" cache-like stuff (the same sort of thing as GLQuake's ms2 files, for example), in AppData, and put the user-config stuff (screenshots, saves, config) in My Games.

That's why you should use SHGetFolderPath - you don't need to worry about that stuff if you just do it the right way.