From 62eeb05519b9fa993f98a540cd8cd812c52fa0e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?L=C3=A9o=20Lam?= <leo@leolam.fr>
Date: Thu, 19 Nov 2020 02:51:56 +0100
Subject: [PATCH] Common: Validate the number of {} fields in format strings

Unfortunately, {fmt} allows passing too many arguments to a format call
without raising any runtime or compile-time error [1].

As this is a common source of bugs since we started migrating to {fmt},
this commit adds some custom logic to validate the number of
replacement fields in format strings in addition to {fmt}'s own checks.

[1] https://github.com/fmtlib/fmt/issues/492
---
 Source/Core/Common/CMakeLists.txt         |  1 +
 Source/Core/Common/Common.vcxproj         |  3 +-
 Source/Core/Common/Common.vcxproj.filters |  3 +-
 Source/Core/Common/FormatUtil.h           | 41 +++++++++++++++++++++++
 Source/Core/Common/Logging/Log.h          | 14 ++++++--
 5 files changed, 58 insertions(+), 4 deletions(-)
 create mode 100644 Source/Core/Common/FormatUtil.h

diff --git a/Source/Core/Common/CMakeLists.txt b/Source/Core/Common/CMakeLists.txt
index cadb12163d..543c4cf9bb 100644
--- a/Source/Core/Common/CMakeLists.txt
+++ b/Source/Core/Common/CMakeLists.txt
@@ -53,6 +53,7 @@ add_library(common
   Flag.h
   FloatUtils.cpp
   FloatUtils.h
+  FormatUtil.h
   FPURoundMode.h
   GekkoDisassembler.cpp
   GekkoDisassembler.h
diff --git a/Source/Core/Common/Common.vcxproj b/Source/Core/Common/Common.vcxproj
index c45edb1b59..07e040b626 100644
--- a/Source/Core/Common/Common.vcxproj
+++ b/Source/Core/Common/Common.vcxproj
@@ -58,6 +58,7 @@
     <ClInclude Include="FileUtil.h" />
     <ClInclude Include="FixedSizeQueue.h" />
     <ClInclude Include="Flag.h" />
+    <ClInclude Include="FormatUtil.h" />
     <ClInclude Include="FPURoundMode.h" />
     <ClInclude Include="GekkoDisassembler.h" />
     <ClInclude Include="GL\GLExtensions\AMD_pinned_memory.h" />
@@ -283,4 +284,4 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>
-</Project>
\ No newline at end of file
+</Project>
diff --git a/Source/Core/Common/Common.vcxproj.filters b/Source/Core/Common/Common.vcxproj.filters
index 0db5b561d0..ec4dc86c9d 100644
--- a/Source/Core/Common/Common.vcxproj.filters
+++ b/Source/Core/Common/Common.vcxproj.filters
@@ -48,6 +48,7 @@
     <ClInclude Include="FixedSizeQueue.h" />
     <ClInclude Include="Flag.h" />
     <ClInclude Include="FloatUtils.h" />
+    <ClInclude Include="FormatUtil.h" />
     <ClInclude Include="FPURoundMode.h" />
     <ClInclude Include="Hash.h" />
     <ClInclude Include="HttpRequest.h" />
@@ -376,4 +377,4 @@
   <ItemGroup>
     <Natvis Include="BitField.natvis" />
   </ItemGroup>
-</Project>
\ No newline at end of file
+</Project>
diff --git a/Source/Core/Common/FormatUtil.h b/Source/Core/Common/FormatUtil.h
new file mode 100644
index 0000000000..3c0bc3f262
--- /dev/null
+++ b/Source/Core/Common/FormatUtil.h
@@ -0,0 +1,41 @@
+// Copyright 2020 Dolphin Emulator Project
+// Licensed under GPLv2+
+// Refer to the license.txt file included.
+
+#pragma once
+
+#include <cstddef>
+#include <string_view>
+
+namespace Common
+{
+constexpr std::size_t CountFmtReplacementFields(std::string_view s)
+{
+  std::size_t count = 0;
+  for (std::size_t i = 0; i < s.size(); ++i)
+  {
+    if (s[i] != '{')
+      continue;
+
+    // If the opening brace is followed by another brace, what we have is
+    // an escaped brace, not a replacement field.
+    if (i + 1 < s.size() && s[i + 1] == '{')
+    {
+      // Skip the second brace.
+      // This ensures that e.g. {{{}}} is counted correctly: when the first brace character
+      // is read and detected as being part of an '{{' escape sequence, the second character
+      // is skipped so the most inner brace (the third character) is not detected
+      // as the end of an '{{' pair.
+      ++i;
+      continue;
+    }
+
+    ++count;
+  }
+  return count;
+}
+
+static_assert(CountFmtReplacementFields("") == 0);
+static_assert(CountFmtReplacementFields("{} test {:x}") == 2);
+static_assert(CountFmtReplacementFields("{} {{}} test {{{}}}") == 2);
+}  // namespace Common
diff --git a/Source/Core/Common/Logging/Log.h b/Source/Core/Common/Logging/Log.h
index 53aee4ec9a..79f4ff6897 100644
--- a/Source/Core/Common/Logging/Log.h
+++ b/Source/Core/Common/Logging/Log.h
@@ -4,8 +4,10 @@
 
 #pragma once
 
+#include <cstddef>
 #include <fmt/format.h>
 #include <string_view>
+#include "Common/FormatUtil.h"
 
 namespace Common::Log
 {
@@ -78,10 +80,13 @@ static const char LOG_LEVEL_TO_CHAR[7] = "-NEWID";
 void GenericLogFmtImpl(LOG_LEVELS level, LOG_TYPE type, const char* file, int line,
                        fmt::string_view format, const fmt::format_args& args);
 
-template <typename S, typename... Args>
+template <std::size_t NumFields, typename S, typename... Args>
 void GenericLogFmt(LOG_LEVELS level, LOG_TYPE type, const char* file, int line, const S& format,
                    const Args&... args)
 {
+  static_assert(NumFields == sizeof...(args),
+                "Unexpected number of replacement fields in format string; did you pass too few or "
+                "too many arguments?");
   GenericLogFmtImpl(level, type, file, line, format,
                     fmt::make_args_checked<Args...>(format, args...));
 }
@@ -141,7 +146,12 @@ void GenericLog(LOG_LEVELS level, LOG_TYPE type, const char* file, int line, con
   do                                                                                               \
   {                                                                                                \
     if (v <= MAX_LOGLEVEL)                                                                         \
-      Common::Log::GenericLogFmt(v, t, __FILE__, __LINE__, FMT_STRING(format), ##__VA_ARGS__);     \
+    {                                                                                              \
+      /* Use a macro-like name to avoid shadowing warnings */                                      \
+      constexpr auto GENERIC_LOG_FMT_N = Common::CountFmtReplacementFields(format);                \
+      Common::Log::GenericLogFmt<GENERIC_LOG_FMT_N>(v, t, __FILE__, __LINE__, FMT_STRING(format),  \
+                                                    ##__VA_ARGS__);                                \
+    }                                                                                              \
   } while (0)
 
 #define ERROR_LOG_FMT(t, ...)                                                                      \