Fix interpretation of 'nan(number)' by the idl_parser (#5810)

* Parser reject "nan(n)" string as it does with nan(n)

* Adjust scalar fuzzer to ignore '$schema' substrings

- Scalar fuzzer ignores '$schema' substrings at the input
- Added 'scalar_debug' target to simplify research of fuzzed cases

* Improve formatting of './tests/fuzzer/CMakeLists.txt'

include/flatbuffers/flatbuffers.h

@@ -1795,8 +1795,7 @@ class FlatBufferBuilder {
       return a.KeyCompareLessThan(&b);
     }
 
-   private:
+    FLATBUFFERS_DELETE_FUNC(StructKeyComparator &operator=(const StructKeyComparator &))
-    StructKeyComparator &operator=(const StructKeyComparator &);
   };
   /// @endcond
 

include/flatbuffers/idl.h

@@ -891,6 +891,7 @@ class Parser : public ParserState {
   FLATBUFFERS_CHECKED_ERROR TokenError();
   FLATBUFFERS_CHECKED_ERROR ParseSingleValue(const std::string *name, Value &e,
                                              bool check_now);
+  FLATBUFFERS_CHECKED_ERROR ParseFunction(const std::string *name, Value &e);
   FLATBUFFERS_CHECKED_ERROR ParseEnumFromString(const Type &type,
                                                 std::string *result);
   StructDef *LookupCreateStruct(const std::string &name,

src/idl_parser.cpp

@@ -436,7 +436,7 @@ CheckedError Parser::Next() {
       default:
         const auto has_sign = (c == '+') || (c == '-');
         // '-'/'+' and following identifier - can be a predefined constant like:
-        // NAN, INF, PI, etc.
+        // NAN, INF, PI, etc or it can be a function name like cos/sin/deg.
         if (IsIdentifierStart(c) || (has_sign && IsIdentifierStart(*cursor_))) {
           // Collect all chars of an identifier:
           const char *start = cursor_ - 1;
@@ -1511,45 +1511,6 @@ CheckedError Parser::ParseMetaData(SymbolTable<Value> *attributes) {
   return NoError();
 }
 
-CheckedError Parser::TryTypedValue(const std::string *name, int dtoken,
-                                   bool check, Value &e, BaseType req,
-                                   bool *destmatch) {
-  bool match = dtoken == token_;
-  if (match) {
-    FLATBUFFERS_ASSERT(*destmatch == false);
-    *destmatch = true;
-    e.constant = attribute_;
-    // Check token match
-    if (!check) {
-      if (e.type.base_type == BASE_TYPE_NONE) {
-        e.type.base_type = req;
-      } else {
-        return Error(
-            std::string("type mismatch: expecting: ") +
-            kTypeNames[e.type.base_type] + ", found: " + kTypeNames[req] +
-            ", name: " + (name ? *name : "") + ", value: " + e.constant);
-      }
-    }
-    // The exponent suffix of hexadecimal float-point number is mandatory.
-    // A hex-integer constant is forbidden as an initializer of float number.
-    if ((kTokenFloatConstant != dtoken) && IsFloat(e.type.base_type)) {
-      const auto &s = e.constant;
-      const auto k = s.find_first_of("0123456789.");
-      if ((std::string::npos != k) && (s.length() > (k + 1)) &&
-          (s[k] == '0' && is_alpha_char(s[k + 1], 'X')) &&
-          (std::string::npos == s.find_first_of("pP", k + 2))) {
-        return Error(
-            "invalid number, the exponent suffix of hexadecimal "
-            "floating-point literals is mandatory: \"" +
-            s + "\"");
-      }
-    }
-
-    NEXT();
-  }
-  return NoError();
-}
-
 CheckedError Parser::ParseEnumFromString(const Type &type,
                                          std::string *result) {
   const auto base_type =
@@ -1638,7 +1599,8 @@ template<typename T> inline void SingleValueRepack(Value &e, T val) {
   if (IsInteger(e.type.base_type)) { e.constant = NumToString(val); }
 }
 #if defined(FLATBUFFERS_HAS_NEW_STRTOD) && (FLATBUFFERS_HAS_NEW_STRTOD > 0)
-// Normilaze defaults NaN to unsigned quiet-NaN(0).
+// Normalize defaults NaN to unsigned quiet-NaN(0) if value was parsed from
+// hex-float literal.
 static inline void SingleValueRepack(Value &e, float val) {
   if (val != val) e.constant = "nan";
 }
@@ -1647,11 +1609,8 @@ static inline void SingleValueRepack(Value &e, double val) {
 }
 #endif
 
-CheckedError Parser::ParseSingleValue(const std::string *name, Value &e,
+CheckedError Parser::ParseFunction(const std::string *name, Value &e) {
-                                      bool check_now) {
+  // Copy name, attribute will be changed on NEXT().
-  // First see if this could be a conversion function:
-  if (token_ == kTokenIdentifier && *cursor_ == '(') {
-    // todo: Extract processing of conversion functions to ParseFunction.
   const auto functionname = attribute_;
   if (!IsFloat(e.type.base_type)) {
     return Error(functionname + ": type of argument mismatch, expecting: " +
@@ -1666,8 +1625,8 @@ CheckedError Parser::ParseSingleValue(const std::string *name, Value &e,
   // calculate with double precision
   double x, y = 0.0;
   ECHECK(atot(e.constant.c_str(), *this, &x));
-    auto func_match = false;
   // clang-format off
+  auto func_match = false;
   #define FLATBUFFERS_FN_DOUBLE(name, op) \
     if (!func_match && functionname == name) { y = op; func_match = true; }
   FLATBUFFERS_FN_DOUBLE("deg", x / kPi * 180);
@@ -1690,9 +1649,58 @@ CheckedError Parser::ParseSingleValue(const std::string *name, Value &e,
   return NoError();
 }
 
-  auto match = false;
+CheckedError Parser::TryTypedValue(const std::string *name, int dtoken,
+                                   bool check, Value &e, BaseType req,
+                                   bool *destmatch) {
+  bool match = dtoken == token_;
+  if (match) {
+    FLATBUFFERS_ASSERT(*destmatch == false);
+    *destmatch = true;
+    e.constant = attribute_;
+    // Check token match
+    if (!check) {
+      if (e.type.base_type == BASE_TYPE_NONE) {
+        e.type.base_type = req;
+      } else {
+        return Error(
+            std::string("type mismatch: expecting: ") +
+            kTypeNames[e.type.base_type] + ", found: " + kTypeNames[req] +
+            ", name: " + (name ? *name : "") + ", value: " + e.constant);
+      }
+    }
+    // The exponent suffix of hexadecimal float-point number is mandatory.
+    // A hex-integer constant is forbidden as an initializer of float number.
+    if ((kTokenFloatConstant != dtoken) && IsFloat(e.type.base_type)) {
+      const auto &s = e.constant;
+      const auto k = s.find_first_of("0123456789.");
+      if ((std::string::npos != k) && (s.length() > (k + 1)) &&
+          (s[k] == '0' && is_alpha_char(s[k + 1], 'X')) &&
+          (std::string::npos == s.find_first_of("pP", k + 2))) {
+        return Error(
+            "invalid number, the exponent suffix of hexadecimal "
+            "floating-point literals is mandatory: \"" +
+            s + "\"");
+      }
+    }
+    NEXT();
+  }
+  return NoError();
+}
+
+CheckedError Parser::ParseSingleValue(const std::string *name, Value &e,
+                                      bool check_now) {
   const auto in_type = e.type.base_type;
+  const auto is_tok_ident = (token_ == kTokenIdentifier);
+  const auto is_tok_string = (token_ == kTokenStringConstant);
+
+  // First see if this could be a conversion function:
+  if (is_tok_ident && *cursor_ == '(') {
+      return ParseFunction(name, e);
+  }
+
   // clang-format off
+  auto match = false;
+
   #define IF_ECHECK_(force, dtoken, check, req)    \
     if (!match && ((check) || IsConstTrue(force))) \
     ECHECK(TryTypedValue(name, dtoken, check, e, req, &match))
@@ -1700,14 +1708,14 @@ CheckedError Parser::ParseSingleValue(const std::string *name, Value &e,
   #define FORCE_ECHECK(dtoken, check, req) IF_ECHECK_(true, dtoken, check, req)
   // clang-format on
 
-  if (token_ == kTokenStringConstant || token_ == kTokenIdentifier) {
+  if (is_tok_ident || is_tok_string) {
     const auto kTokenStringOrIdent = token_;
     // The string type is a most probable type, check it first.
     TRY_ECHECK(kTokenStringConstant, in_type == BASE_TYPE_STRING,
                BASE_TYPE_STRING);
 
     // avoid escaped and non-ascii in the string
-    if (!match && (token_ == kTokenStringConstant) && IsScalar(in_type) &&
+    if (!match && is_tok_string && IsScalar(in_type) &&
         !attr_is_trivial_ascii_string_) {
       return Error(
           std::string("type mismatch or invalid value, an initializer of "
@@ -1735,11 +1743,20 @@ CheckedError Parser::ParseSingleValue(const std::string *name, Value &e,
     }
     // Parse a float/integer number from the string.
     if (!match) check_now = true;  // Re-pack if parsed from string literal.
-    if (!match && (token_ == kTokenStringConstant) && IsScalar(in_type)) {
+    // A "scalar-in-string" value needs extra checks.
-      // remove trailing whitespaces from attribute_
+    if (!match && is_tok_string && IsScalar(in_type)) {
-      auto last = attribute_.find_last_not_of(' ');
+      // Strip trailing whitespaces from attribute_.
-      if (std::string::npos != last)  // has non-whitespace
+      auto last_non_ws = attribute_.find_last_not_of(' ');
-        attribute_.resize(last + 1);
+      if (std::string::npos != last_non_ws)
+        attribute_.resize(last_non_ws + 1);
+      if (IsFloat(e.type.base_type)) {
+        // The functions strtod() and strtof() accept both 'nan' and
+        // 'nan(number)' literals. While 'nan(number)' is rejected by the parser
+        // as an unsupported function if is_tok_ident is true.
+        if (attribute_.find_last_of(')') != std::string::npos) {
+          return Error("invalid number: " + attribute_);
+        }
+      }
     }
     // Float numbers or nan, inf, pi, etc.
     TRY_ECHECK(kTokenStringOrIdent, IsFloat(in_type), BASE_TYPE_FLOAT);

tests/fuzzer/CMakeLists.txt

@@ -1,42 +1,76 @@
 cmake_minimum_required(VERSION 3.9)
 
 set(CMAKE_VERBOSE_MAKEFILE ON)
-
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 
 project(FlatBuffersFuzzerTests)
 
-set(CMAKE_CXX_FLAGS
+option(BUILD_DEBUGGER "Compile a debugger with main() and without libFuzzer" OFF)
-  "${CMAKE_CXX_FLAGS} -std=c++14 -Wall -pedantic -Werror -Wextra -Wno-unused-parameter -fsigned-char")
 
-set(CMAKE_CXX_FLAGS
+if(NOT DEFINED FLATBUFFERS_MAX_PARSING_DEPTH)
-  "${CMAKE_CXX_FLAGS} -g -fsigned-char -fno-omit-frame-pointer")
+  # Force checking of RecursionError in the test
-
+  set(FLATBUFFERS_MAX_PARSING_DEPTH 8)
-# Typical slowdown introduced by MemorySanitizer (memory) is 3x.
-# '-fsanitize=address' not allowed with '-fsanitize=memory'
-if(YES)
-  set(CMAKE_CXX_FLAGS
-    "${CMAKE_CXX_FLAGS} -fsanitize=fuzzer,address,undefined")
-else()
-  set(CMAKE_CXX_FLAGS
-    "${CMAKE_CXX_FLAGS} -fsanitize=fuzzer,memory,undefined -fsanitize-memory-track-origins=2")
 endif()
+message(STATUS "FLATBUFFERS_MAX_PARSING_DEPTH: ${FLATBUFFERS_MAX_PARSING_DEPTH}")
 
-set(CMAKE_CXX_FLAGS
+# Usage '-fsanitize=address' doesn't allowed with '-fsanitize=memory'.
-  "${CMAKE_CXX_FLAGS} -fsanitize-coverage=edge,trace-cmp")
+# MemorySanitizer will not work out-of-the-box, and will instead report false
-
+# positives coming from uninstrumented code. Need to re-build both C++ standard
-# enable link-time optimisation
+# library: https://github.com/google/sanitizers/wiki/MemorySanitizerLibcxxHowTo
-# set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -flto")
+option(USE_MSAN "Use MSAN instead of ASASN" OFF)
-
-# https://llvm.org/docs/Passes.html
-# save IR to see call graph
-# make one bitcode file:> llvm-link *.bc -o out.bc
-# print call-graph:> opt out.bc -analyze -print-callgraph &> callgraph.txt
-# set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -save-temps -flto")
 
+# Use Clang linker.
 set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fuse-ld=lld")
 
+# add_link_options(-stdlib=libc++)
+
+add_compile_options(
+  # -stdlib=libc++ # Use Clang libc++ instead of GNU.
+  -std=c++14
+  -Wall
+  -pedantic
+  -Werror
+  -Wextra
+  -Wno-unused-parameter
+  -fsigned-char
+  -fno-omit-frame-pointer
+  -g # Generate source-level debug information
+  # -flto # enable link-time optimisation
+)
+
+# https://llvm.org/docs/Passes.html save IR to see call graph make one bitcode
+# file:> llvm-link *.bc -o out.bc print call-graph:> opt out.bc -analyze -print-
+# callgraph &> callgraph.txt set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -save-temps
+# -flto")
+
+# A special target with fuzzer+sanitizer flags.
+add_library(fuzzer_config INTERFACE)
+
+target_compile_options(
+  fuzzer_config
+  INTERFACE
+    -fsanitize-coverage=edge,trace-cmp
+    $<$<BOOL:NOT ${USE_MSAN}>:
+      -fsanitize=fuzzer,undefined,address
+    >
+    $<$<BOOL:${USE_MSAN}>:
+      -fsanitize=fuzzer,undefined,memory
+      -fsanitize-memory-track-origins=2
+    >
+)
+
+target_link_libraries(
+  fuzzer_config
+  INTERFACE
+    $<$<BOOL:NOT ${USE_MSAN}>:
+      -fsanitize=fuzzer,undefined,address
+    >
+    $<$<BOOL:${USE_MSAN}>:
+      -fsanitize=fuzzer,undefined,memory
+    >
+)
+
 set(FLATBUFFERS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../")
 
 set(FlatBuffers_Library_SRCS
@@ -60,30 +94,45 @@ set(FlatBuffers_Library_SRCS
 
 include_directories(${FLATBUFFERS_DIR}/include)
 include_directories(${FLATBUFFERS_DIR}/tests)
-add_library(flatbuffers STATIC ${FlatBuffers_Library_SRCS})
 
-# FLATBUFFERS_ASSERT should assert in Release as well.
+add_library(flatbuffers_fuzzed STATIC ${FlatBuffers_Library_SRCS})
-# Redefine FLATBUFFERS_ASSERT macro definition.
+# Use PUBLIC to force 'fuzzer_config' for all dependent targets
-# Declare as PUBLIC to cover asserts in all included header files.
+target_link_libraries(flatbuffers_fuzzed PUBLIC fuzzer_config)
-target_compile_definitions(flatbuffers PUBLIC
-   FLATBUFFERS_ASSERT=fuzzer_assert_impl)
-target_compile_definitions(flatbuffers PUBLIC
-   FLATBUFFERS_ASSERT_INCLUDE="${CMAKE_CURRENT_SOURCE_DIR}/fuzzer_assert.h")
 
-if(NOT DEFINED FLATBUFFERS_MAX_PARSING_DEPTH)
+# FLATBUFFERS_ASSERT should assert in Release as well. Redefine
-  # Force checking of RecursionError in the test
+# FLATBUFFERS_ASSERT macro definition. Declare as PUBLIC to cover asserts in all
-  set(FLATBUFFERS_MAX_PARSING_DEPTH 8)
+# included header files.
-endif()
+target_compile_definitions(
-message(STATUS "FLATBUFFERS_MAX_PARSING_DEPTH: ${FLATBUFFERS_MAX_PARSING_DEPTH}")
+  flatbuffers_fuzzed
-target_compile_definitions(flatbuffers PRIVATE FLATBUFFERS_MAX_PARSING_DEPTH=8)
+  PUBLIC
+    FLATBUFFERS_ASSERT=fuzzer_assert_impl
+    FLATBUFFERS_ASSERT_INCLUDE="${CMAKE_CURRENT_SOURCE_DIR}/fuzzer_assert.h"
+  PRIVATE
+    FLATBUFFERS_MAX_PARSING_DEPTH=${FLATBUFFERS_MAX_PARSING_DEPTH}
+)
 
 # Setup fuzzer tests.
 
 add_executable(scalar_fuzzer flatbuffers_scalar_fuzzer.cc)
-target_link_libraries(scalar_fuzzer PRIVATE flatbuffers)
+target_link_libraries(scalar_fuzzer PRIVATE flatbuffers_fuzzed)
 
 add_executable(parser_fuzzer flatbuffers_parser_fuzzer.cc)
-target_link_libraries(parser_fuzzer PRIVATE flatbuffers)
+target_link_libraries(parser_fuzzer PRIVATE flatbuffers_fuzzed)
 
 add_executable(verifier_fuzzer flatbuffers_verifier_fuzzer.cc)
-target_link_libraries(verifier_fuzzer PRIVATE flatbuffers)
+target_link_libraries(verifier_fuzzer PRIVATE flatbuffers_fuzzed)
+
+# Build debugger for weird cases found with fuzzer.
+if(BUILD_DEBUGGER)
+  add_library(flatbuffers_nonfuzz STATIC ${FlatBuffers_Library_SRCS})
+  target_compile_definitions(
+    flatbuffers_nonfuzz
+    PUBLIC
+      FLATBUFFERS_ASSERT=fuzzer_assert_impl
+      FLATBUFFERS_ASSERT_INCLUDE="${CMAKE_CURRENT_SOURCE_DIR}/fuzzer_assert.h"
+    PRIVATE
+      FLATBUFFERS_MAX_PARSING_DEPTH=${FLATBUFFERS_MAX_PARSING_DEPTH}
+  )
+  add_executable(scalar_debug flatbuffers_scalar_fuzzer.cc scalar_debug.cpp)
+  target_link_libraries(scalar_debug PRIVATE flatbuffers_nonfuzz)
+endif(BUILD_DEBUGGER)

tests/fuzzer/flatbuffers_parser_fuzzer.cc

@@ -26,7 +26,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   if (size < 3) return 0;
   const uint8_t flags = data[0];
   // normalize to ascii alphabet
-  const int extra_rep_number = data[1] >= '0' ? (data[1] - '0') : 0;
+  const int extra_rep_number =
+      std::max(5, (data[1] < '0' ? (data[1] - '0') : 0));
   data += 2;
   size -= 2;  // bypass
 

tests/fuzzer/flatbuffers_scalar_fuzzer.cc

@@ -101,8 +101,8 @@ class IntegerRegex : public RegexMatcher {
     static const std::vector<std::regex> re_list = {
       std::regex{ R"(^[-+]?[0-9]+$)", std::regex_constants::optimize },
 
-      std::regex{
+      std::regex{ R"(^[-+]?0[xX][0-9a-fA-F]+$)",
-          R"(^[-+]?0[xX][0-9a-fA-F]+$)", std::regex_constants::optimize }
+                  std::regex_constants::optimize }
     };
     return MatchRegexList(input, re_list);
   }
@@ -117,8 +117,8 @@ class UIntegerRegex : public RegexMatcher {
   bool MatchNumber(const std::string &input) const override {
     static const std::vector<std::regex> re_list = {
       std::regex{ R"(^[+]?[0-9]+$)", std::regex_constants::optimize },
-      std::regex{
+      std::regex{ R"(^[+]?0[xX][0-9a-fA-F]+$)",
-          R"(^[+]?0[xX][0-9a-fA-F]+$)", std::regex_constants::optimize },
+                  std::regex_constants::optimize },
       // accept -0 number
       std::regex{ R"(^[-](?:0[xX])?0+$)", std::regex_constants::optimize }
     };
@@ -216,7 +216,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   if (size < 3) return 0;
   const uint8_t flags = data[0];
   // normalize to ascii alphabet
-  const int extra_rep_number = data[1] >= '0' ? (data[1] - '0') : 0;
+  const int extra_rep_number =
+      std::max(5, (data[1] < '0' ? (data[1] - '0') : 0));
   data += 2;
   size -= 2;  // bypass
 
@@ -232,6 +233,9 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   // We reject this by transform "/* text */ 12345" to "@* text */ 12345".
   BreakSequence(input, "//", '@');  // "//" -> "@/"
   BreakSequence(input, "/*", '@');  // "/*" -> "@*"
+  // { "$schema: "text" } is exceptional case.
+  // This key:value ignored by the parser. Numbers can not have $.
+  BreakSequence(input, "$schema", '@');  // "$schema" -> "@schema"
   // Break all known scalar functions (todo: add them to regex?):
   for (auto f : { "deg", "rad", "sin", "cos", "tan", "asin", "acos", "atan" }) {
     BreakSequence(input, f, '_');  // ident -> ident

tests/fuzzer/scalar_debug.cpp

@@ -0,0 +1,28 @@
+#include <iostream>
+#include "flatbuffers/util.h"
+
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int main(int argc, char *argv[]) {
+  if (argc < 2) {
+    std::cerr << "Usage: scalar_debug <path to fuzzer crash file>\n";
+    return 0;
+  }
+  std::string crash_file_name(argv[1]);
+  std::string crash_file_data;
+  auto done =
+      flatbuffers::LoadFile(crash_file_name.c_str(), true, &crash_file_data);
+  if (!done) {
+    std::cerr << "Can not load file: '" << crash_file_name << "'";
+    return -1;
+  }
+  if (crash_file_data.size() < 3) {
+    std::cerr << "Invalid file data: '" << crash_file_data << "'";
+    return -2;
+  }
+  auto rc = LLVMFuzzerTestOneInput(
+      reinterpret_cast<const uint8_t *>(crash_file_data.data()),
+      crash_file_data.size());
+  std::cout << "LLVMFuzzerTestOneInput finished with code " << rc;
+  return rc;
+}

tests/test.cpp

@@ -2019,6 +2019,9 @@ void InvalidFloatTest() {
   TestError("table T { F:float; } root_type T; { F:0x0 }", invalid_msg);
   TestError("table T { F:float; } root_type T; { F:-0x. }", invalid_msg);
   TestError("table T { F:float; } root_type T; { F:0x. }", invalid_msg);
+  TestError("table T { F:float; } root_type T; { F:0Xe }", invalid_msg);
+  TestError("table T { F:float; } root_type T; { F:\"0Xe\" }", invalid_msg);
+  TestError("table T { F:float; } root_type T; { F:\"nan(1)\" }", invalid_msg);
   // eE not exponent in hex-float!
   TestError("table T { F:float; } root_type T; { F:0x0.0e+ }", invalid_msg);
   TestError("table T { F:float; } root_type T; { F:0x0.0e- }", invalid_msg);