FormatStringSan (bValidateFormatStrings) doesn't emit NotEnoughSpecifiers if the format string ends with specifier

Development Pipeline & Plugins
, ,
1

The repro steps for the issue should be pretty simple. I found that FormatStringSan.h basically checks in this way when it still has remaining args to check against the rest of the format string:

  1. If the current char is ‘\0’ and we’re not in a dynamic length specifier, return NotEnoughSpecifiers.
  2. If we’re not in a dynamic length specifier, increment char pos until we get to a ‘%’ or ‘\0’.
  3. If we got to a ‘%’, increment pos.
  4. If the current char is now ‘\0’,
    1. If the previous char was ‘%’, return IncompleteFormatSpecifierOrUnescapedPercent.
    2. Else return Ok. // problem is here

So in this case, we’re still verifying an argument, but we reached the end of the format string without finding another specifier. I fixed this on my end by moving the NotEnoughSpecifiers block to be after the block following it. Now the logic looks like this:

  1. If we’re not in a dynamic length specifier, increment char pos until we get to a ‘%’ or ‘\0’.
  2. If we got to a ‘%’, increment pos.
  3. If the current char is now ‘\0’ and we’re not in a dynamic length specifier, return NotEnoughSpecifiers.
  4. If the current char is now ‘\0’,
    1. If the previous char was ‘%’, return IncompleteFormatSpecifierOrUnescapedPercent.
    2. Else return Ok. // problem is here

This might not be the most elegant, I probably still need to rework the NotEnoughSpecifiers vs. IncompleteFormatSpecifierOrUnescapedPercent a little bit. But you get the idea.

When I look back at the change that introduced NotEnoughSpecifiers, it was submitted with fixes to all the strings that had issues at the time. Validating my theory, every string that was fixed ends with a specifier, except for one that was an empty string. In addition, all the Catch2 unit tests checking for this functionality end with specifiers. After fixing it, 3 of those tests break: 2 of them seem to accidentally use $d instead of %d, and one predates the check for NotEnoughSpecifiers and seems to be specifically checking that an extra argument is acceptable. Notably, this test continued passing because it does not end with a specifier.

I started fixing all the issues this found, but I got to about 75 and decided to write the ticket before I kept going. Needless to say, it will not be a trivial thing to address, but I hope it will still be addressed when time permits.

2

Steps to Reproduce
In a target with bValidateFormatStrings set to true:

  1. Add code like UE_LOG(TEXT(“%d %d”), 1, 2, 3);
  2. Compile, and get an error like “missing format specifier (Is there a missing ‘%’ somewhere?)”
  3. Change the line to UE_LOG(TEXT("%d %d "), 1, 2, 3);
    1. Notice the space after the last specifier
  4. Compile the code, and see that there is no longer an error.
3

Hi Matt,

Cheers for the heads up. I think your suggested fix is almost correct, but I’d move it to the middle of the next block, i.e.:

if (!bInsideFormatSpec)
{
	while (*Fmt != (CharType)'\0' && *Fmt != (CharType)'%')
	{
		++Fmt;
	}
 
	if (Fmt[0] == (CharType)'\0')
	{
		return { EFormatStringSanStatus::NotEnoughSpecifiers, CurArgPos };
	}
 
	if (*Fmt == (CharType)'%')
	{
		++Fmt;
	}
}

That way, if you have a trailing % you get the IncompleteFormatSpecifierOrUnescapedPercent like you said.

I’ll see about getting both it and the fallout fixed - seems like there’s quite a lot of real bugs that it’s revealing.

Thanks!

Steve