vsnprintf does not always nullterminate

[JannesN]

Consider this code: char NumStr[32]; int Round = 116; bx::snprintf(NumStr, 32, "@[s:GUIText|a:000000FF]Round %i", Round);

In this case the buffer fits exactly the string WITHOUT the null terminator! In this special case the resulting string is not null terminated.

This is the writing code in vsnprintf: `StaticMemoryBlockWriter writer(_out, uint32_t(_max) );

		Error err;
		va_list argListCopy;
		va_copy(argListCopy, _argList);
		int32_t size = write(&writer, _format, argListCopy, &err);
		va_end(argListCopy);

		if (err.isOk() )
		{
			size += write(&writer, '\0', &err);
			return size - 1 /* size without '\0' terminator */;
		}
		else
		{
			_out[_max-1] = '\0';
		}`

In order for it to work correctly the writing of the null terminator has to be moved out of the if statement. Since err.isOk() will be true when the buffer was fully written!

On top of that it looks a bit to me like a return statement is missing in the else part?

[JannesN]

Here is a simpler example of the bug: char Buff[2]; bx::snprintf(Buff, BX_COUNTOF(Buff), "aa"); BX_ASSERT(bx::strLen(Buff) < 2, "This is bad.");

As you can see it ends up not being null terminated.

[JannesN]

Suggested fix:

	int32_t vsnprintf(char* _out, int32_t _max, const char* _format, va_list _argList)
	{
		if (1 < _max)
		{
			StaticMemoryBlockWriter writer(_out, uint32_t(_max) );

			Error err;
			va_list argListCopy;
			va_copy(argListCopy, _argList);
			int32_t size = write(&writer, _format, argListCopy, &err);
			va_end(argListCopy);
			write(&writer, '\0', &err);//Moved this out of the error check! Did not increase size.

			if (err.isOk() )
			{
				return size;//Subtraction no longer needed
			}
			else
			{
				_out[_max-1] = '\0';
			}
		}

		Error err;
		SizerWriter sizer;
		va_list argListCopy;
		va_copy(argListCopy, _argList);
		int32_t total = write(&sizer, _format, argListCopy, &err);
		va_end(argListCopy);

		return total;
	}

[bkaradzic]

Can you create failing unit test?

[JannesN]

The best to expand is the vsnprintf truncated test. It indeed fails.

TEST_CASE("vsnprintf truncated", "Truncated output buffer.")
{
	char buffer5[5]; // fit
	REQUIRE(4 == bx::snprintf(buffer5, BX_COUNTOF(buffer5), "abvg") );
	REQUIRE(0 == bx::strCmp(buffer5, "abvg") );

	char buffer7[7]; // truncate
	REQUIRE(10 == bx::snprintf(buffer7, BX_COUNTOF(buffer7), "Ten chars!") );
	REQUIRE(0  == bx::strCmp(buffer7, "Ten ch") );

	REQUIRE(6 == bx::snprintf(buffer7, BX_COUNTOF(buffer7), "Seven?!"));//Truncate exactly by one character
	REQUIRE(buffer7[BX_COUNTOF(buffer7)-1] == '\0');//Should be null terminated!
	REQUIRE(0 == bx::strCmp(buffer7, "Seven!"));//Six characters should be written
}