From 769bf456b40a6bb24408d374f0707965819b83f1 Mon Sep 17 00:00:00 2001 From: amyspark <13498015+amyspark@users.noreply.github.com> Date: Wed, 8 Dec 2021 11:22:04 -0300 Subject: [PATCH] Implement locale-agnostic number parsing (#1496) This commit adds support for parsing numbers without being influenced by the current system locale. We implement a from_chars shim that forwards the call to strto*_l along with a statically initialized locale constant. Fixes AcademySoftwareFoundation#297 Fixes AcademySoftwareFoundation#379 Fixes AcademySoftwareFoundation#1322 Co-Authored-By: Patrick Hodoul Signed-off-by: L. E. Segovia <13498015+amyspark@users.noreply.github.com> --- share/cmake/utils/CompilerFlags.cmake | 4 + src/OpenColorIO/CMakeLists.txt | 1 + src/OpenColorIO/ParseUtils.cpp | 20 ++- src/OpenColorIO/fileformats/FileFormatHDL.cpp | 12 +- .../fileformats/FileFormatIridasLook.cpp | 12 +- .../fileformats/FileFormatSpi1D.cpp | 56 +++++- .../fileformats/FileFormatSpi3D.cpp | 39 +++- .../fileformats/xmlutils/XMLReaderUtils.h | 25 +-- src/utils/CMakeLists.txt | 9 + src/utils/NumberUtils.h | 166 ++++++++++++++++++ 10 files changed, 304 insertions(+), 40 deletions(-) create mode 100644 src/utils/NumberUtils.h diff --git a/share/cmake/utils/CompilerFlags.cmake b/share/cmake/utils/CompilerFlags.cmake index 1fe5591c3a..9e56d55ae8 100644 --- a/share/cmake/utils/CompilerFlags.cmake +++ b/share/cmake/utils/CompilerFlags.cmake @@ -25,6 +25,10 @@ if(USE_MSVC) # Note: Do not use /Wall (i.e. /W4) which adds 'informational' warnings. set(PLATFORM_COMPILE_FLAGS "${PLATFORM_COMPILE_FLAGS} /W3") + # Do enable C4701 (Potentially uninitialized local variable 'name' used), which is level 4. + # This is because strtoX-based from_chars leave the value variable unmodified. + set(PLATFORM_COMPILE_FLAGS "${PLATFORM_COMPILE_FLAGS} /we4701") + if(OCIO_WARNING_AS_ERROR) set(PLATFORM_COMPILE_FLAGS "${PLATFORM_COMPILE_FLAGS} /WX") endif() diff --git a/src/OpenColorIO/CMakeLists.txt b/src/OpenColorIO/CMakeLists.txt index aba3cb9133..959ad11304 100755 --- a/src/OpenColorIO/CMakeLists.txt +++ b/src/OpenColorIO/CMakeLists.txt @@ -226,6 +226,7 @@ target_link_libraries(OpenColorIO ${OCIO_HALF_LIB} pystring::pystring sampleicc::sampleicc + utils::from_chars utils::strings yaml-cpp ) diff --git a/src/OpenColorIO/ParseUtils.cpp b/src/OpenColorIO/ParseUtils.cpp index 6a3976d4ff..59b4f71d46 100644 --- a/src/OpenColorIO/ParseUtils.cpp +++ b/src/OpenColorIO/ParseUtils.cpp @@ -1,6 +1,7 @@ // SPDX-License-Identifier: BSD-3-Clause // Copyright Contributors to the OpenColorIO Project. +#include #include #include #include @@ -11,6 +12,7 @@ #include "ParseUtils.h" #include "Platform.h" #include "utils/StringUtils.h" +#include "utils/NumberUtils.h" namespace OCIO_NAMESPACE @@ -524,6 +526,7 @@ static constexpr int DOUBLE_DECIMALS = 16; std::string FloatToString(float value) { std::ostringstream pretty; + pretty.imbue(std::locale::classic()); pretty.precision(FLOAT_DECIMALS); pretty << value; return pretty.str(); @@ -534,6 +537,7 @@ std::string FloatVecToString(const float * fval, unsigned int size) if(size<=0) return ""; std::ostringstream pretty; + pretty.imbue(std::locale::classic()); pretty.precision(FLOAT_DECIMALS); for(unsigned int i=0; i> x)) + float x = NAN; + const auto result = NumberUtils::from_chars(str, str + strlen(str), x); + if (result.ec != std::errc()) { return false; } @@ -565,6 +569,7 @@ bool StringToInt(int * ival, const char * str, bool failIfLeftoverChars) if(!ival) return false; std::istringstream i(str); + i.imbue(std::locale::classic()); char c=0; if (!(i >> *ival) || (failIfLeftoverChars && i.get(c))) return false; return true; @@ -573,6 +578,7 @@ bool StringToInt(int * ival, const char * str, bool failIfLeftoverChars) std::string DoubleToString(double value) { std::ostringstream pretty; + pretty.imbue(std::locale::classic()); pretty.precision(DOUBLE_DECIMALS); pretty << value; return pretty.str(); @@ -583,6 +589,7 @@ std::string DoubleVecToString(const double * val, unsigned int size) if (size <= 0) return ""; std::ostringstream pretty; + pretty.imbue(std::locale::classic()); pretty.precision(DOUBLE_DECIMALS); for (unsigned int i = 0; i &floatArray, const StringUtils::Stri for(unsigned int i=0; i> x)) + float x = NAN; + const char *str = lineParts[i].c_str(); + const auto result = NumberUtils::from_chars(str, str + lineParts[i].size(), x); + if (result.ec != std::errc()) { return false; } diff --git a/src/OpenColorIO/fileformats/FileFormatHDL.cpp b/src/OpenColorIO/fileformats/FileFormatHDL.cpp index c71e3dd516..6a9ddcefd5 100755 --- a/src/OpenColorIO/fileformats/FileFormatHDL.cpp +++ b/src/OpenColorIO/fileformats/FileFormatHDL.cpp @@ -37,6 +37,7 @@ #include "ParseUtils.h" #include "transforms/FileTransform.h" #include "utils/StringUtils.h" +#include "utils/NumberUtils.h" namespace OCIO_NAMESPACE @@ -182,16 +183,11 @@ readLuts(std::istream& istream, } else if(inlut) { - // StringToFloat was far slower, for 787456 values: - // - StringToFloat took 3879 (avg nanoseconds per value) - // - stdtod took 169 nanoseconds - char* endptr = 0; - float v = static_cast(strtod(word.c_str(), &endptr)); + float v{}; + const auto result = NumberUtils::from_chars(word.c_str(), word.c_str() + word.size(), v); - if(!*endptr) + if (result.ec == std::errc()) { - // Since each word should contain a single - // float value, the pointer should be null lutValues[lutname].push_back(v); } else diff --git a/src/OpenColorIO/fileformats/FileFormatIridasLook.cpp b/src/OpenColorIO/fileformats/FileFormatIridasLook.cpp index 0f83f5b346..7402efd095 100755 --- a/src/OpenColorIO/fileformats/FileFormatIridasLook.cpp +++ b/src/OpenColorIO/fileformats/FileFormatIridasLook.cpp @@ -16,6 +16,7 @@ #include "pystring/pystring.h" #include "transforms/FileTransform.h" #include "utils/StringUtils.h" +#include "utils/NumberUtils.h" /* @@ -416,19 +417,18 @@ class XMLParserHelper std::string size_raw = std::string(s, len); std::string size_clean = pystring::strip(size_raw, "'\" "); // strip quotes and space - char* endptr = 0; - int size_3d = static_cast(strtol(size_clean.c_str(), &endptr, 10)); + long int size_3d{}; + + const auto result = NumberUtils::from_chars(size_clean.c_str(), size_clean.c_str() + size_clean.size(), size_3d); - if (*endptr) + if (result.ec != std::errc()) { - // strtol didn't consume entire string, there was - // remaining data, thus did not contain a single integer std::ostringstream os; os << "Invalid LUT size value: '" << size_raw; os << "'. Expected quoted integer"; pImpl->Throw(os.str().c_str()); } - pImpl->m_lutSize = size_3d; + pImpl->m_lutSize = static_cast(size_3d); } else if (pImpl->m_data) { diff --git a/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp b/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp index b30af3795d..39723d0775 100755 --- a/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp +++ b/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp @@ -1,7 +1,9 @@ // SPDX-License-Identifier: BSD-3-Clause // Copyright Contributors to the OpenColorIO Project. +#include #include +#include #include #include @@ -13,7 +15,7 @@ #include "Platform.h" #include "transforms/FileTransform.h" #include "utils/StringUtils.h" - +#include "utils/NumberUtils.h" /* Version 1 @@ -124,10 +126,26 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, } else if(StringUtils::StartsWith(headerLine, "From")) { - if (sscanf(lineBuffer, "From %f %f", &from_min, &from_max) != 2) + char fromMinS[64] = ""; + char fromMaxS[64] = ""; +#ifdef _WIN32 + if (sscanf_s(lineBuffer, "From %s %s", fromMinS, 64, fromMaxS, 64) != 2) +#else + if (sscanf(lineBuffer, "From %s %s", fromMinS, fromMaxS) != 2) +#endif { ThrowErrorMessage("Invalid 'From' Tag", currentLine, headerLine); } + else + { + const auto fromMinAnswer = NumberUtils::from_chars(fromMinS, fromMinS + 64, from_min); + const auto fromMaxAnswer = NumberUtils::from_chars(fromMaxS, fromMaxS + 64, from_max); + + if (fromMinAnswer.ec != std::errc() || fromMaxAnswer.ec != std::errc()) + { + ThrowErrorMessage("Invalid 'From' Tag", currentLine, headerLine); + } + } } else if(StringUtils::StartsWith(headerLine, "Components")) { @@ -179,7 +197,6 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, int lineCount=0; - StringUtils::StringVec inputLUT; std::vector values; while (istream.good()) @@ -192,10 +209,17 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, if (line.length() != 0) { - inputLUT = StringUtils::SplitByWhiteSpaces(StringUtils::Trim(lineBuffer)); values.clear(); - if (!StringVecToFloatVec(values, inputLUT) - || components != (int)values.size()) + + char inputLUT[4][64] = {"", "", "", ""}; +#ifdef _WIN32 + if (sscanf_s(lineBuffer, "%s %s %s %63s", inputLUT[0], 64, + inputLUT[1], 64, inputLUT[2], 64, inputLUT[3], + 64) != components) +#else + if (sscanf(lineBuffer, "%s %s %s %63s", inputLUT[0], + inputLUT[1], inputLUT[2], inputLUT[3]) != components) +#endif { std::ostringstream os; os << "Malformed LUT line. Expecting a "; @@ -209,6 +233,26 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, ThrowErrorMessage("Too many entries found", currentLine, ""); } + values.resize(components); + + for (int i = 0; i < components; i++) + { + float v = NAN; + const auto result = NumberUtils::from_chars(inputLUT[i], inputLUT[i] + 64, v); + + if (result.ec != std::errc()) + { + std::ostringstream os; + os << "Malformed LUT line. Could not convert component"; + os << i << " to a floating point number."; + + ThrowErrorMessage("Malformed LUT line", currentLine, + line); + } + + values[i] = v; + } + // If 1 component is specified, use x1 x1 x1. if (components == 1) { diff --git a/src/OpenColorIO/fileformats/FileFormatSpi3D.cpp b/src/OpenColorIO/fileformats/FileFormatSpi3D.cpp index f69ba492f2..3ad240689d 100755 --- a/src/OpenColorIO/fileformats/FileFormatSpi3D.cpp +++ b/src/OpenColorIO/fileformats/FileFormatSpi3D.cpp @@ -12,6 +12,7 @@ #include "Platform.h" #include "transforms/FileTransform.h" #include "utils/StringUtils.h" +#include "utils/NumberUtils.h" /* @@ -131,7 +132,7 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, // Parse table int index = 0; int rIndex, gIndex, bIndex; - float redValue, greenValue, blueValue; + float redValue = NAN, greenValue = NAN, blueValue = NAN; int entriesRemaining = rSize * gSize * bSize; Array & lutArray = lut3d->getArray(); @@ -141,10 +142,42 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, { istream.getline(lineBuffer, MAX_LINE_SIZE); - if (sscanf(lineBuffer, "%d %d %d %f %f %f", + char redValueS[64] = ""; + char greenValueS[64] = ""; + char blueValueS[64] = ""; + +#ifdef _WIN32 + if (sscanf(lineBuffer, + "%d %d %d %s %s %s", + &rIndex, &gIndex, &bIndex, + redValueS, 64, + greenValueS, 64, + blueValueS, 64) == 6) +#else + if (sscanf(lineBuffer, "%d %d %d %s %s %s", &rIndex, &gIndex, &bIndex, - &redValue, &greenValue, &blueValue) == 6) + redValueS, greenValueS, blueValueS) == 6) +#endif { + const auto redValueAnswer = NumberUtils::from_chars(redValueS, redValueS + 64, redValue); + const auto greenValueAnswer = NumberUtils::from_chars(greenValueS, greenValueS + 64, greenValue); + const auto blueValueAnswer = NumberUtils::from_chars(blueValueS, blueValueS + 64, blueValue); + + if (redValueAnswer.ec != std::errc() + || greenValueAnswer.ec != std::errc() + || blueValueAnswer.ec != std::errc()) + { + std::ostringstream os; + os << "Error parsing .spi3d file ("; + os << fileName; + os << "). "; + os << "Data is invalid. "; + os << "A color value is specified ("; + os << redValueS << " " << greenValueS << " " << blueValueS; + os << ") that cannot be parsed as a floating-point triplet."; + throw Exception(os.str().c_str()); + } + bool invalidIndex = false; if (rIndex < 0 || rIndex >= rSize || gIndex < 0 || gIndex >= gSize diff --git a/src/OpenColorIO/fileformats/xmlutils/XMLReaderUtils.h b/src/OpenColorIO/fileformats/xmlutils/XMLReaderUtils.h index 56162321ba..9a813a2551 100644 --- a/src/OpenColorIO/fileformats/xmlutils/XMLReaderUtils.h +++ b/src/OpenColorIO/fileformats/xmlutils/XMLReaderUtils.h @@ -13,6 +13,8 @@ #include #include "MathUtils.h" +#include "utils/StringUtils.h" +#include "utils/NumberUtils.h" #include "Platform.h" @@ -156,16 +158,17 @@ void ParseNumber(const char * str, size_t startPos, size_t endPos, T & value) const char * startParse = str + startPos; double val = 0.0f; - char * endParse = nullptr; - - // The strtod expects a C string and str might not be null terminated. - // However since strtod will stop parsing when it encounters characters - // that it cannot convert to a number, in practice it does not need to - // be null terminated. - // C++11 version of strtod processes NAN & INF ASCII values. - val = strtod(startParse, &endParse); + + size_t adjustedStartPos = startPos; + size_t adjustedEndPos = endPos; + + FindSubString(startParse, endPos - startPos, adjustedStartPos, adjustedEndPos); + + const auto result = NumberUtils::from_chars(startParse + adjustedStartPos, startParse + adjustedEndPos, val); + value = (T)val; - if (endParse == startParse) + + if (result.ec == std::errc::invalid_argument) { std::string fullStr(str, endPos); std::string parsedStr(startParse, endPos - startPos); @@ -187,10 +190,10 @@ void ParseNumber(const char * str, size_t startPos, size_t endPos, T & value) << TruncateString(fullStr.c_str(), endPos, 100) << "'."; throw Exception(oss.str().c_str()); } - else if (endParse != str + endPos) + else if (result.ptr != str + endPos) { // Number is followed by something. - std::string fullStr(str, startPos + (endParse - startParse)); + std::string fullStr(str, endPos); std::string parsedStr(startParse, endPos - startPos); std::ostringstream oss; oss << "ParserNumber: '" diff --git a/src/utils/CMakeLists.txt b/src/utils/CMakeLists.txt index fa1acf31a3..ab029d222e 100755 --- a/src/utils/CMakeLists.txt +++ b/src/utils/CMakeLists.txt @@ -15,3 +15,12 @@ configure_file("Half.h.in" "Half.h" @ONLY) set_property(TARGET ${OCIO_HALF_LIB} APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${CMAKE_CURRENT_BINARY_DIR}/.." ) + +# from_chars shim (via strtod_l) + +add_library(utils::from_chars INTERFACE IMPORTED GLOBAL) + +set_target_properties(utils::from_chars PROPERTIES + INTERFACE_INCLUDE_DIRECTORIES "${CMAKE_CURRENT_SOURCE_DIR}/.." +) + diff --git a/src/utils/NumberUtils.h b/src/utils/NumberUtils.h new file mode 100644 index 0000000000..d0a7471f21 --- /dev/null +++ b/src/utils/NumberUtils.h @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: BSD-3-Clause +// Copyright Contributors to the OpenColorIO Project. + +#ifndef INCLUDED_NUMBERUTILS_H +#define INCLUDED_NUMBERUTILS_H + +#if defined(_MSC_VER) +#define really_inline __forceinline +#else +#define really_inline inline __attribute__((always_inline)) +#endif + +#include +#include +#include +#include + +namespace OCIO_NAMESPACE +{ +namespace NumberUtils +{ + +struct Locale +{ +#ifdef _WIN32 + Locale() : local(_create_locale(LC_ALL, "C")) + { + } + ~Locale() + { + _free_locale(local); + } + _locale_t local; +#else + Locale() : local(newlocale(LC_ALL_MASK, "C", NULL)) + { + } + ~Locale() + { + freelocale(local); + } + locale_t local; +#endif +}; + +struct from_chars_result +{ + const char *ptr; + std::errc ec; +}; + +static const Locale loc; + +really_inline from_chars_result from_chars(const char *first, const char *last, double &value) noexcept +{ + errno = 0; + if (!first || !last || first == last) + { + return {first, std::errc::invalid_argument}; + } + + char * endptr = nullptr; + + double +#ifdef _WIN32 + tempval = _strtod_l(first, &endptr, loc.local); +#else + tempval = ::strtod_l(first, &endptr, loc.local); +#endif + + if (errno != 0) + { + return {first + (endptr - first), std::errc::result_out_of_range}; + } + else if (endptr == first) + { + return {first, std::errc::invalid_argument}; + } + else if (endptr <= last) + { + value = tempval; + return {first + (endptr - first), {}}; + } + else + { + return {first, std::errc::argument_out_of_domain}; + } +} + +really_inline from_chars_result from_chars(const char *first, const char *last, float &value) noexcept +{ + errno = 0; + if (!first || !last || first == last) + { + return {first, std::errc::invalid_argument}; + } + + char *endptr = nullptr; + + float +#ifdef _WIN32 + tempval = _strtof_l(first, &endptr, loc.local); +#elif __APPLE__ + // On OSX, strtod_l is for some reason drastically faster than strtof_l. + tempval = static_cast(::strtod_l(first, &endptr, loc.local)); +#else + tempval = ::strtof_l(first, &endptr, loc.local); +#endif + + if (errno != 0) + { + return {first + (endptr - first), std::errc::result_out_of_range}; + } + else if (endptr == first) + { + return {first, std::errc::invalid_argument}; + } + else if (endptr <= last) + { + value = tempval; + return {first + (endptr - first), {}}; + } + else + { + return {first, std::errc::argument_out_of_domain}; + } +} + +really_inline from_chars_result from_chars(const char *first, const char *last, long int &value) noexcept +{ + errno = 0; + if (!first || !last || first == last) + { + return {first, std::errc::invalid_argument}; + } + + char *endptr = nullptr; + + long int +#ifdef _WIN32 + tempval = _strtol_l(first, &endptr, 0, loc.local); +#else + tempval = ::strtol_l(first, &endptr, 0, loc.local); +#endif + + if (errno != 0) + { + return {first + (endptr - first), std::errc::result_out_of_range}; + } + else if (endptr == first) + { + return {first, std::errc::invalid_argument}; + } + else if (endptr <= last) + { + value = tempval; + return {first + (endptr - first), {}}; + } + else + { + return {first, std::errc::argument_out_of_domain}; + } +} +} // namespace NumberUtils +} // namespace OCIO_NAMESPACE +#endif // INCLUDED_NUMBERUTILS_H