Make tinyformat noexcept: Use default error handling (assert(0 && reason)) on incorrect format strings #13391

Make tinyformat noexcept: Use tinyformat's default error handling (assert(0 && reason);) instead of throwing tinyformat::format_error on incorrect format strings.

The only place where tinyformat::format_error was handled was in LogPrintf(...) where it was handled to generate output which is equivalent to what assert(0 && reason); provides.

Incorrect format strings are best handled explicitly locally in a "fail fast" manner. No need for propagation.

Since tinyformat is used extensively via strprintf(…) large parts of the code base are switched from an implicit "might throw tinyformat::format_error" to "will not throw" from a compiler/static analyzer perspective with this change.

Reducing the number of unnecessary exceptions thrown increases the signal-to-noise for humans when analyzing potential issues introduced by uncaught exceptions.

These were the conditions that threw tinyformat::format_error prior to this commit:

$ git grep TINYFORMAT_ERROR
src/tinyformat.h:// Error handling: Define TINYFORMAT_ERROR to customize the error handling for
src/tinyformat.h:#define TINYFORMAT_ERROR(reasonString) throw tinyformat::format_error(reasonString)
src/tinyformat.h:#ifndef TINYFORMAT_ERROR
src/tinyformat.h:#   define TINYFORMAT_ERROR(reason) assert(0 && reason)
src/tinyformat.h:        TINYFORMAT_ERROR("tinyformat: Cannot convert from argument type to "
src/tinyformat.h:        TINYFORMAT_ERROR("tinyformat: Not enough conversion specifiers in format string");
src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: Not enough arguments to read variable width");
src/tinyformat.h:                TINYFORMAT_ERROR("tinyformat: Not enough arguments to read variable precision");
src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: the %a and %A conversion specs "
src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: %n conversion spec not supported");
src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: Conversion spec incorrectly "
src/tinyformat.h:            TINYFORMAT_ERROR("tinyformat: Not enough format arguments");
src/tinyformat.h:        TINYFORMAT_ERROR("tinyformat: Too many conversion specifiers in format string");

Tend towards NACK:

• Changes to tinyformat.h should be made upstream.
• A mistake in log message formatting should not crash the entire process. This was the case in the past, and resulted in near DoS vulnerabilities. The current state where a warning is logged instead is on purpose.

This basically reverts b651270cd6bfdd6d7c4acf04d1a00c03bf09f03a (#9963) and I disagree with that strongly.

@laanwj Thanks for quick feedback. I've now reverted the tinyformat.h change to tinyformat code from upstream (the noexcept addition).

Could you describe the issue that resulted in a near DoS vulnerability? As I read it #9963 was more about developer ergonomics? :-)

Could a middle ground be to make strprintf(…) noexcept and keep LogPrintf(...) as is?

None of the callers of strprintf(…) handle tinyformat::format_error from what I can see. Also, it is the strprintf(…) calls that create all the noise when I'm analyzing uncaught exceptions :-)

Could you describe the issue that resulted in a near DoS vulnerability? As I read it #9963 was more about developer ergonomics? :-)

I think we intentionally didn't mention the issue there. It's very simply: have an under-tested error path in the network code with an invalid LogPrintf() spec. Anything that triggers this will crash the process. A little loggable offense turns into a full DoS vector.

Could a middle ground be to make strprintf(…) noexcept and keep LogPrintf(...) as is?

I think that would be a better idea. If the output isn't just used for logging, it's a more serious error in any case.

@laanwj I see. Makes perfect sense. Closing!

@practicalswift at least for the number of arguments versus number of specifiers, we could have a linter. Though it's harder to do than most of the checks.

@laanwj Good idea! I'll look into that when I'm done with my exception handling work :-)