#709 Adapt snprintf behaviour to always write 0 byte

[Dominik Maier]

The current implementation of snprintf is not complying with most posix implementations.
Instead of copying n - 1 bytes to the given buffer, it copies n bytes, then stops and returns -1.
If users rely on the 0 termination usually supplied by this function (and don't check their error codes...), this can easily break code.

To give an example:

char buf[4];
int n = snprintf(buf, sizeof(buf), "test");
printf("%s", buf);

on arch prints "tes" (0 terminated) whereas in mingw-w64 it would not 0 terminate the buffer but instead print test and whatever else is lying around in memory.

While, of course, adhering more closely to posix implementations is nice to have but, also, more importantly, it might lead to security relevant bugs in some cases.

[NightStrike]

C99 says that this behavior is undefined. IIRC, modern GCC warns on this
condition.

On Thu, Feb 22, 2018 at 2:31 PM, Dominik Mair domenukk@users.sourceforge.net wrote:

---

• [bugs:#709] https://sourceforge.net/p/mingw-w64/bugs/709/ Adapt
  snprintf behaviour to always write 0 byte*

Status: open
Group: v1.0 (example)
Created: Thu Feb 22, 2018 07:31 PM UTC by Dominik Mair
Last Updated: Thu Feb 22, 2018 07:31 PM UTC
Owner: nobody

The current implementation of snprintf is not complying with most posix
implementations.
Instead of copying n - 1 bytes to the given buffer, it copies n bytes,
then stops and returns -1.
If users rely on the 0 termination usually supplied by this function (and
don't check their error codes...), this can easily break code.

To give an example:

char buf[4];
int n = snprintf(buf, sizeof(buf), "test");
printf("%s", buf);

on arch prints "tes" (0 terminated) whereas in mingw-w64 it would not 0
terminate the buffer but instead print test and whatever else is lying
around in memory.

While, of course, adhering more closely to posix implementations is nice
to have but, also, more importantly, it might lead to security relevant
bugs in some cases.

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/mingw-w64/bugs/709/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

[Dominik Maier]

The C99 draft at http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf states

7.19.6.5 The snprintf function
[...]
Otherwise, output characters beyond the n-1st are discarded rather than being written to the array, and a null character is written at the end of the characters actually written into the array
[...]
The snprintf function returns the number of characters that would have been written had n been sufficiently large

So according to this, the last character in the buffer should indeed be a '\0' and the return value should be the string length it would have written, given a large enough buffer.

[NightStrike]

Ah, I was looking at sprintf, not snprintf. Oops. You are right, we are
not in compliance with the spec.

On Thu, Feb 22, 2018 at 9:07 PM, Dominik Maier domenukk@users.sourceforge.net wrote:

The C99 draft at http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
states

7.19.6.5 The snprintf function
[...]
Otherwise, output characters beyond the n-1st are discarded rather than
being written to the array, and a null character is written at the end of
the characters actually written into the array
[...]
The snprintf function returns the number of characters that would have
been written had n been sufficiently large

So according to this, the last character in the buffer should indeed be a
'\0' and the return value should be the string length it would have
written, given a large enough buffer.

---

• [bugs:#709] https://sourceforge.net/p/mingw-w64/bugs/709/ Adapt
  snprintf behaviour to always write 0 byte*

Status: open
Group: v1.0 (example)
Created: Thu Feb 22, 2018 07:31 PM UTC by Dominik Maier
Last Updated: Thu Feb 22, 2018 07:31 PM UTC
Owner: nobody

The current implementation of snprintf is not complying with most posix
implementations.
Instead of copying n - 1 bytes to the given buffer, it copies n bytes,
then stops and returns -1.
If users rely on the 0 termination usually supplied by this function (and
don't check their error codes...), this can easily break code.

To give an example:

char buf[4];
int n = snprintf(buf, sizeof(buf), "test");
printf("%s", buf);

on arch prints "tes" (0 terminated) whereas in mingw-w64 it would not 0
terminate the buffer but instead print test and whatever else is lying
around in memory.

While, of course, adhering more closely to posix implementations is nice
to have but, also, more importantly, it might lead to security relevant
bugs in some cases.

---

Sent from sourceforge.net because you indicated interest in
https://sourceforge.net/p/mingw-w64/bugs/709/

To unsubscribe from further messages, please visit
https://sourceforge.net/auth/subscriptions/

[Dominik Maier]

Digging deeper into the issue, I found out mingw-w64-crt use Microsofts _vsnprintf function instead of vsnprintf[1], which does not add the zero-char for input size n >= strlen.
This is true for the snprintf and vsnprintf functions alike.

In use cases in which programs only check the return value for error codes, not for truncation, this can lead to out of bounds reads in subsequent functions since -1 will only be returned for any input larger than the buffer, but not for strlen == n.

Instead, if possible, vsnprintf should be used [2].
A quick mitigation would, alternatively, be to always insert the '\0' at s[n -1], although still not posix compliant.

Thanks for the quick response.

[1] https://sourceforge.net/p/mingw-w64/mingw-w64/ci/v5.x/tree/mingw-w64-crt/stdio/vsnprintf.c#l12
[2] https://msdn.microsoft.com/en-us/library/1kt27hek.aspx

[Old Wolf 2]

Note - the code works correctly (i.e. according to ISO C spec) if #define __USE_MINGW_ANSI_STDIO 1 is active ; I enable this in all my code , never understood what the value was in replicating Microsoft's bugs by default...

BTW not sure why people are quoting C99 draft when C11 has been out for 7 years now

[Jonathan Yong]

Fixed in dc3b2e2bfa9b5a4fcee6f0123047ecc5a6a35d1f.