macOS: warning: 'sprintf' is deprecated

[knev]

On macOS, this scrolls along the screen constantly, since the functions are used everywhere.

/Users/dev/Project-@knev/dhewm3/neo/idlib/Str.h: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
        sprintf( text, "%f", b );
        ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk/usr/include/stdio.h:188:1: note: 'sprintf' has been explicitly marked deprecated here
__deprecated_msg("This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead.")
^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk/usr/include/sys/cdefs.h:215:48: note: expanded from macro '__deprecated_msg'
        #define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg)))

According to /Users/dev/Project-@knev/dhewm3/neo/idlib/Str.h line 42, snprintf should NOT be used for platform compatibility. What approach did you want to take on this? Just leave the warning (annoying) or perhaps use a pre-processor #if defined(MACOS_X) || defined(__APPLE__) test just for macOS? Something else?

[DanielGibson]

It's used 337 times in the code, so removing it would be a lot of work, and often it's completely harmless, like in the example you get the warning about: There it's used to print a float into a buffer, which is known to be big enough to hold any result of formatting a float as string with "%f".

I'd suggest disabling the deprecation warning, I'm sure there's a compiler flag or #define for that.

IIRC current versions of GCC (possible when used together with Linux' glibc?) have more helpful warnings on this, that take the size of the buffer printed to into account (if it's known) and actually check how big the string could get based on the arguments, and only warn if it could actually overflow.

[knev]

I like a tidy compile process, so I'm willing to do the work if you tell me how you would like to handle it. I'm not sure if you want 337 preprocessor checks...

#if defined(MACOS_X) || defined(__APPLE__)
	vsnprintf(text, MAX_STRING_CHARS, str, ap);
#else
	vsprintf(text, str, ap);
#endif

Another idea: Since vsnprintf and snprintf are already redefined, it would be possible to create a macro that takes the same amount of args as vsnprintf and snprintf, but then on non-mac platforms map it to just vsprintf and sprintf.

If you don't want to touch it, then I might disable deprecation, yes. I would think it is a shame we can't get a clean build on mac though.

[DanielGibson]

I'd prefer not to change it, because all those little changes (esp. in game.dll code) can make porting mods (or code from other source ports for non-game.dll-code) harder.

If you absolutely want to give it a try, dhewm3 already has D3_snprintf() and D3_vsnprintf() as (v)snprintf() replacements that should work on all platforms and behave like the versions in C99 (ensuring \0-termination, returning how many chars would've been used if the buffer had been big enough)

[knev]

Thanks. Good point. I don't think I have a good argument against that. I will take a look at it tomorrow. Cheers!

[DanielGibson]

A stray thought, it might be possible to write a custom sprintf() that's overloaded for arrays with static lengths and does length checking on that. Like Microsoft does in their (non-standard) implementations: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/sprintf-sprintf-l-swprintf-swprintf-l-swprintf-l?view=msvc-170

Something like

template<size_t bufLen>
int sprintf(char (&buf)[bufLen], const char* format, ...)
{
	int ret = 0;
	va_list argptr;
	va_start( argptr, format );
	ret = D3_vsnprintfC99(buf, bufLen, format, argptr);
	va_end( argptr );
	return ret;
}

(I didn't test this, but char (&buf)[bufLen] is the syntax used in the Microsoft docs linked above, so I hope this is the correct way to do this.) Then you should only get your deprecation warnings for the places where sprintf() is not called with a fixed-size array (but maybe a pointer from a function argument or whatever), and hopefully that will be relatively few instances and maybe those can be fixed by using D3_snprintf() explicitly. (and you can do basically the same for vsprintf(), of course)

And I just noticed, also keep in mind that there already are overloaded versions of sprintf() and vsprintf() that take a idStr& as first argument, those are already safe.

And lots of instances of (v)sprintf() usage is in the Windows-only tool code, so if you grep for it and wonder why you don't get deprecation warnings for source files in tools/, that's why (though there is some tool code that's crossplatform, mostly in tools/compilers/ I think)

[knev]

For posterity: disabling the deprecated warning on mac is ...

if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang")
	add_compile_options(-pipe)
	add_compile_options(-Wall)

// ...

	# TODO fix these warnings
	add_compile_options(-Wno-sign-compare)
	add_compile_options(-Wno-switch)
	add_compile_options(-Wno-format-security)

	add_compile_options(-Wno-deprecated-declarations) // this line
// ...

[jayaddison]

FWIW: I'm encountering similar format-security compile-time issues when attempting to package quadrilateralcowboy for Debian. The changeset required to achieve a successful compile (pending testing...) there weren't huge (see blendogames/quadrilateralcowboy#4).

I don't know whether those changes actually improve any security properties of the build (it's largely adding "%s" printf arguments, which seems close to a no-op.. but I'm no expert for C-like-languages).

[DanielGibson]

Hmm maybe I'd even merge (most of) those changes, but they're pretty different - removing all sprintf() calls (even if they're not even theoretically dangerous) would be (even) more invasive than that

[knev]

One "solution" is to redefine sprintf/vsprintf to D3_ specific functions local to the idStr compilation unit. Without changing any functionality, this means the deprecation warnings are just printed once on screen instead of a rolling list on all compilation units. Here is the implementation: https://github.com/dhewm/dhewm3/compare/master...knev:dhewm3:deprecated_sprintf

Let me know if you find this an ok approach. I might be able to go further with this with more overloading as @DanielGibson suggested.