fwknop icon indicating copy to clipboard operation
fwknop copied to clipboard

fwknop uses unsafe _snprintf() on Windows

Open khorben opened this issue 6 years ago • 0 comments

In common/common.h, line 93:

 87 /* Some hoops for accommodating Windows
 88 */
 89 #ifdef WIN32
 90   #include <io.h>
 91   #define strcasecmp    _stricmp
 92   #define strncasecmp   _strnicmp
 93   #define snprintf              _snprintf

and in lib/fko_common.h, line 66:

 62 #ifdef WIN32
 63   #include <io.h>
 64   #define strcasecmp  _stricmp
 65   #define strncasecmp _strnicmp
 66   #define snprintf    _snprintf

Windows builds of fwknop are set to use _snprintf() when snprintf() is normally used. This is discouraged, and even dangerous, since the implementation of _snprintf() on Windows is not behaving as expected: it does not automatically terminate strings upon overflows. See https://msdn.microsoft.com/en-us/library/2ts7cx93.aspx for a complete explanation. Perhaps unbeknownst to this fact, as it happens many snprintf() calls in the code are fortunately performed this way:

char buf[BUF_SIZE] = {0}; //initialization with all-zeros apparently

snprintf(buf, sizeof(buf) - 1, "%s", input_string);

On UNIX this wastes the space for one character, since snprintf() will truncate the buffer at sizeof(buf) - 2 as expected. On Windows luckily even though the buffer was not truncated there, the buffer initialization will apparently guarantee the presence of a terminating character. Now that Windows supports a C99-conformant implementation of snprintf(), it should be possible to remove this crude hack. In the meantime every snprintf() call should probably be reviewed for correctness there.

khorben avatar Jul 24 '18 16:07 khorben