GNOME Bugzilla – Bug 756691
Substitution of _snprintf for snprintf in Windows may cause buffer overruns
Last modified: 2016-02-25 14:10:32 UTC
As Microsoft only recently implemented the C++11 compliant version of snprintf() it has been common practice to replace it with _snprintf() on Windows as below. win32config.h:70-77 #include <direct.h> #if defined(_MSC_VER) && (_MSC_VER < 1900) || defined(__MINGW32__) #define mkdir(p,m) _mkdir(p) #define snprintf _snprintf #if _MSC_VER < 1500 #define vsnprintf(b,c,f,a) _vsnprintf(b,c,f,a) #endif #endif Unfortunately _snprintf() and is not the same as snprintf() as it doesn't guarantee null termination of the resulting buffer. _vsnprintf() has the same problem. I don't know if this causes any actual problems in the library, but if it does then they'll be extremely unpleasant buffer overruns on the Windows platform. snprintf() should be wrapped with an actually equivalent function, not substituted for one that's nearly-but-not-quite the same. (Noticed because I've been fixing the problem in my own code as that was picking up this wrong definition from win32config.h) Further references: https://randomascii.wordpress.com/2013/04/03/stop-using-strncpy-already/ http://stackoverflow.com/questions/2915672/snprintf-and-visual-studio-2010 http://stackoverflow.com/questions/3976306/using-snprintf-in-a-cross-platform-application I'll attach my fix when I get to it. It's based on the Stack Overflow suggestion in question 2915672 but I need to verify some behaviour before putting it forward as a fix (notably the return value if the destination buffer is NULL)
Created attachment 313643 [details] C++ program to test different snprintf() replacements. Simple C++ program to unit test behaviour of different snprintf() replacement strategies on Windows. Change the #define of USING_SNPRINTF to whichever strategy you want: SNPRINTF_WRAPPER_FUNCTION - Tests the proposed wrapper function from Stack Overflow. Passes all tests. SNPRINTF_NAIVE_DEFINE - Tests #define snprintf _snprinf. This fails, because _snprintf() is not the same as snprintf() SNPRINTF_BUILTIN - Uses the compiler provided snprintf() function, if available. Passes all tests. The first two were tested in Visual Studio 2005, and the last using Cygwin/gcc.
The following code properly replaces snprintf() on Windows: inline int vsnprintf(char *outBuf, size_t size, const char *format, va_list ap) { // _vsnprintf_s does almost what we want from vsnprintf, but it doesn't // return the number of characters that would have been printed if the // buffer had been large enough. Instead it returns -1 if _TRUNCATE is // specified // const int VSNPRINTF_S_TRUNCATE_FAILURE_CODE = -1; int count = VSNPRINTF_S_TRUNCATE_FAILURE_CODE; if (size) count = _vsnprintf_s(outBuf, size, _TRUNCATE, format, ap); if (count == VSNPRINTF_S_TRUNCATE_FAILURE_CODE) count = _vscprintf(format, ap); return count; } inline int snprintf(char *buffer, size_t count, const char *format, ... ) { va_list ap; va_start(ap, format); int result = vsnprintf(buffer, count, format, ap); va_end(ap); return result; }
While attempting to merge the fix with the win32config.h file I've uncovered another horror. MS provide a vsnprintf() in some cases where there's no snprintf(), however it's not the same as the standard vsnprintf(). It returns -1 if truncation occurs, not the number of characters that would have been printed if the buffer was large enough. This means that the *fixed* snprintf() will break if relying on the broken MS vsprintf() implementation. The two replacements have to go together, with the *fixed* vsnprintf() overriding the default MS version, or it all breaks horribly again. I'll modify the test cases and extend as appropriate. Note that I'm also not going to be able to test my fix in all versions of Visual Studio.
Created attachment 313649 [details] Updated test function now includes MS variant vsnprintf() Updated test program to include additional strategy to test: SNPRINTF_WITH_MS_VSNPRINTF : Uses the modified snprintf() but calls the MS provided vsnprintf() instead of the modified vsnprintf(). Fails tests (expected) Tried on VS2005. Note that this strategy compiles, runs and passes tests via Cygwin/gcc but that's because it's using gcc's vsnprintf() and that works.
Created attachment 313654 [details] [review] Patch to fix incompatible MS version of snprintf()/vsnprintf() Patch file to make snprintf() and vsnprintf()on Windows platforms perform the same as on other platforms. Note - will need regression testing on a variety of platforms and compilers to make sure that it still builds correctly (Tested on VS2012) Double Note - Unsure what will happen with VS2015. I believe this implements the correct snprintf() behaviour in the default library now, but it will doubtless still contain the non-conformant vsnprintf() for backwards (in)compatibility. Signing off now. Thanks for listening.
Fixed in master: https://git.gnome.org/browse/libxslt/commit/?id=e75b5da121cdc67cb2303b1e2b77d5dd1cdf2784