Latest release (7.1.0) causes binskim errors

[Oskorei]

We're running binskim (https://github.com/microsoft/binskim) in our build process, which started to report errors with the latest fmt.lib release (7.1.0).

error BA2014: 'update.dll' is a C or C++ binary built with function(s) (fmt::v7::detail::dragonbox::to_decimal<float>;fmt::v7::detail::dragonbox::to_decimal<double>) that disable the stack protector. The stack protector (/GS) is a security feature of the compiler which makes it more difficult to exploit stack buffer overflow memory corruption vulnerabilities. Disabling the stack protector, even on a function-by-function basis, is disallowed by SDL policy. To resolve this issue, remove occurrences of __declspec(safebuffers) from your code. If the additional code inserted by the stack protector has been shown in profiling to cause a significant performance problem for your application, attempt to move stack buffer modifications out of the hot path of execution to allow the compiler to avoid inserting stack protector checks in these locations rather than disabling the stack protector altogether.

[vitaut]

@jk-jeon, do we actually need FMT_SAFEBUFFERS?

[jk-jeon]

Not really, it's there just for the performance reason.

But I have no idea why MSVC ever wants to put buffer overrun checks there. Dragonbox doesn't even use any pointers at all, except for the static cache array, which seems to be not the reason why MSVC thinks it is vulnerable; probably this is some bug related to intrinsics I guess. But well, I'm not a security expert, and if possible I hope someone who knows better than me about buffer overrun attacks could review the code.

Security buffer check itself indeed does not cause any significant performance issue, but the real issue is that the feature seems to interfere with the inlining decision, thus can cause a pretty measurable performance drop. Again I guess this should be a bug, but well, there is nothing we can do with it so I disabled the (apparently, at least to me) needless checks.

[vitaut]

Security buffer check itself indeed does not cause any significant performance issue, but the real issue is that the feature seems to interfere with the inlining decision, thus can cause a pretty measurable performance drop.

Would it make sense to use always inline instead?

[jk-jeon]

What I recall is that somehow the compiler sometimes rejected to inline a function marked with __forceinline if I remove __declspec(safebuffers) (I honestly didn't understand what's written here; but according to my bare understanding, inlining should not be prevented by this feature, so I consider it as a bug). But it might not be the case for the code we have now. Maybe I should compare the generated codes and then get back to you!

[jk-jeon]

It turns out __declspec(safebuffers) does not affect inlining decision for the code we have now, at least on my platform (Win10 64-bit, VS16.7.6). It just puts a little bit of code at the beginning and the end of to_decimal, which does not affect the performance that much. I think we can just get rid of FMT_SAFEBUFFERS 😁

[vitaut]

Thanks for checking, @jk-jeon. Removed in 112755c.