From 55029bbf533b26ddb2ea35acfdb4e1c7434a401e Mon Sep 17 00:00:00 2001 From: Addisu Taddese Date: Thu, 4 Jun 2020 13:47:17 -0500 Subject: [PATCH] Make parsing of values syntactically more strict with bad values generating an error (#244) Currently, libsdformat silently ignores syntax errors when parsing values. For example, bad 0 0 0 0 0 or 0.1 0.2 0.3 0.4 0.5 are both bad values for the tag, but the parsed values are 0 0 0 0 0 0 and 0.1 0.2 0.3 0.4 0.5 0 respectively. With this PR, libsdformat will generate an error for such values. --- Changelog.md | 3 + include/sdf/Types.hh | 6 ++ src/Param.cc | 101 +++++++++++++++++++-------------- src/Types.cc | 12 ++-- src/parser_TEST.cc | 61 ++++++++++++++++++++ test/sdf/bad_syntax_double.sdf | 13 +++++ test/sdf/bad_syntax_pose.sdf | 10 ++++ test/sdf/bad_syntax_vector.sdf | 7 +++ test/sdf/box_bad_test.world | 2 +- 9 files changed, 168 insertions(+), 47 deletions(-) create mode 100644 test/sdf/bad_syntax_double.sdf create mode 100644 test/sdf/bad_syntax_pose.sdf create mode 100644 test/sdf/bad_syntax_vector.sdf diff --git a/Changelog.md b/Changelog.md index 8bdd7df86..e20540798 100644 --- a/Changelog.md +++ b/Changelog.md @@ -16,6 +16,9 @@ ### libsdformat 10.0.0 (202X-XX-XX) +1. Make parsing of values syntactically more strict with bad values generating an error + * [Pull request 244](https://github.com/osrf/sdformat/pull/244) + 1. Don't install deprecated parser_urdf.hh header file, fix cmake warning about newline file, fix cmake warning about newlines. * [Pull request 276](https://github.com/osrf/sdformat/pull/276) diff --git a/include/sdf/Types.hh b/include/sdf/Types.hh index 7e0b8ea7d..607f7712f 100644 --- a/include/sdf/Types.hh +++ b/include/sdf/Types.hh @@ -74,6 +74,12 @@ namespace sdf SDFORMAT_VISIBLE std::string trim(const char *_in); + /// \brief Trim leading and trailing whitespace from a string. + /// \param[in] _in The string to trim. + /// \return A string containing the trimmed value. + SDFORMAT_VISIBLE + std::string trim(const std::string &_in); + /// \brief check if two values are equal, within a tolerance /// \param[in] _a the first value /// \param[in] _b the second value diff --git a/src/Param.cc b/src/Param.cc index 0ab69a43a..17fbb5f0b 100644 --- a/src/Param.cc +++ b/src/Param.cc @@ -16,6 +16,7 @@ */ #include +#include #include #include #include @@ -271,6 +272,29 @@ std::string Param::GetDefaultAsString() const return ss.str(); } +////////////////////////////////////////////////// +/// \brief Helper function for Param::ValueFromString +/// \param[in] _input Input string. +/// \param[in] _key Key of the parameter, used for error message. +/// \param[out] _value This will be set with the parsed value. +/// \return True if parsing succeeded. +template +bool ParseUsingStringStream(const std::string &_input, const std::string &_key, + ParamPrivate::ParamVariant &_value) +{ + StringStreamClassicLocale ss(_input); + T _val; + ss >> _val; + if (ss.fail()) + { + sdferr << "Unknown error. Unable to set value [" << _input << " ] for key[" + << _key << "]\n"; + return false; + } + _value = _val; + return true; +} + ////////////////////////////////////////////////// bool Param::ValueFromString(const std::string &_value) { @@ -278,9 +302,9 @@ bool Param::ValueFromString(const std::string &_value) // comma for decimal position instead of a dot, making the conversion // to fail. See bug #60 for more information. Force to use always C setlocale(LC_NUMERIC, "C"); - - std::string tmp(_value); - std::string lowerTmp = lowercase(_value); + std::string trimmed = sdf::trim(_value); + std::string tmp(trimmed); + std::string lowerTmp = lowercase(trimmed); // "true" and "false" doesn't work properly if (lowerTmp == "true") @@ -335,11 +359,8 @@ bool Param::ValueFromString(const std::string &_value) } else if (this->dataPtr->typeName == "uint64_t") { - StringStreamClassicLocale ss(tmp); - std::uint64_t u64tmp; - - ss >> u64tmp; - this->dataPtr->value = u64tmp; + return ParseUsingStringStream(tmp, this->dataPtr->key, + this->dataPtr->value); } else if (this->dataPtr->typeName == "unsigned int") { @@ -357,66 +378,62 @@ bool Param::ValueFromString(const std::string &_value) else if (this->dataPtr->typeName == "sdf::Time" || this->dataPtr->typeName == "time") { - StringStreamClassicLocale ss(tmp); - sdf::Time timetmp; - - ss >> timetmp; - this->dataPtr->value = timetmp; + return ParseUsingStringStream(tmp, this->dataPtr->key, + this->dataPtr->value); } else if (this->dataPtr->typeName == "ignition::math::Color" || this->dataPtr->typeName == "color") { - StringStreamClassicLocale ss(tmp); - ignition::math::Color colortmp; + // The insertion operator (>>) expects 4 values, but the last value (the + // alpha) is optional. We first try to parse assuming the alpha is + // specified. If that fails, we append the default value of alpha to the + // string and try to parse again. + bool result = ParseUsingStringStream( + tmp, this->dataPtr->key, this->dataPtr->value); - ss >> colortmp; - this->dataPtr->value = colortmp; + if (!result) + { + ignition::math::Color colortmp; + return ParseUsingStringStream( + tmp + " " + std::to_string(colortmp.A()), this->dataPtr->key, + this->dataPtr->value); + } + else + return true; } else if (this->dataPtr->typeName == "ignition::math::Vector2i" || this->dataPtr->typeName == "vector2i") { - StringStreamClassicLocale ss(tmp); - ignition::math::Vector2i vectmp; - - ss >> vectmp; - this->dataPtr->value = vectmp; + return ParseUsingStringStream( + tmp, this->dataPtr->key, this->dataPtr->value); } else if (this->dataPtr->typeName == "ignition::math::Vector2d" || this->dataPtr->typeName == "vector2d") { - StringStreamClassicLocale ss(tmp); - ignition::math::Vector2d vectmp; - - ss >> vectmp; - this->dataPtr->value = vectmp; + return ParseUsingStringStream( + tmp, this->dataPtr->key, this->dataPtr->value); } else if (this->dataPtr->typeName == "ignition::math::Vector3d" || this->dataPtr->typeName == "vector3") { - StringStreamClassicLocale ss(tmp); - ignition::math::Vector3d vectmp; - - ss >> vectmp; - this->dataPtr->value = vectmp; + return ParseUsingStringStream( + tmp, this->dataPtr->key, this->dataPtr->value); } else if (this->dataPtr->typeName == "ignition::math::Pose3d" || this->dataPtr->typeName == "pose" || this->dataPtr->typeName == "Pose") { - StringStreamClassicLocale ss(tmp); - ignition::math::Pose3d posetmp; - - ss >> posetmp; - this->dataPtr->value = posetmp; + if (!tmp.empty()) + { + return ParseUsingStringStream( + tmp, this->dataPtr->key, this->dataPtr->value); + } } else if (this->dataPtr->typeName == "ignition::math::Quaterniond" || this->dataPtr->typeName == "quaternion") { - StringStreamClassicLocale ss(tmp); - ignition::math::Quaterniond quattmp; - - ss >> quattmp; - this->dataPtr->value = quattmp; + return ParseUsingStringStream( + tmp, this->dataPtr->key, this->dataPtr->value); } else { diff --git a/src/Types.cc b/src/Types.cc index dfbbc74af..a0cd1636e 100644 --- a/src/Types.cc +++ b/src/Types.cc @@ -53,17 +53,21 @@ std::vector split(const std::string &_str, ////////////////////////////////////////////////// std::string trim(const char *_in) { - std::string str(_in); + return sdf::trim(std::string(_in)); +} - const size_t strBegin = str.find_first_not_of(" \t"); +////////////////////////////////////////////////// +std::string trim(const std::string &_in) +{ + const size_t strBegin = _in.find_first_not_of(" \t"); if (strBegin == std::string::npos) { return ""; } - const size_t strRange = str.find_last_not_of(" \t") - strBegin + 1; + const size_t strRange = _in.find_last_not_of(" \t") - strBegin + 1; - return str.substr(strBegin, strRange); + return _in.substr(strBegin, strRange); } ///////////////////////////////////////////////// diff --git a/src/parser_TEST.cc b/src/parser_TEST.cc index c524cc51b..8c4d825cb 100644 --- a/src/parser_TEST.cc +++ b/src/parser_TEST.cc @@ -472,6 +472,67 @@ TEST(Parser, NameUniqueness) } } +///////////////////////////////////////////////// +/// Check that _a contains _b +static bool contains(const std::string &_a, const std::string &_b) +{ + return _a.find(_b) != std::string::npos; +} + +///////////////////////////////////////////////// +TEST(Parser, SyntaxErrorInValues) +{ + std::string pathBase = PROJECT_SOURCE_PATH; + pathBase += "/test/sdf"; + + // Capture sdferr output + std::stringstream buffer; + auto old = std::cerr.rdbuf(buffer.rdbuf()); + +#ifdef _WIN32 + sdf::Console::Instance()->SetQuiet(false); +#endif + + { + std::string path = pathBase +"/bad_syntax_pose.sdf"; + sdf::SDFPtr sdf(new sdf::SDF()); + sdf::init(sdf); + + sdf::readFile(path, sdf); + EXPECT_PRED2(contains, buffer.str(), + "Unable to set value [bad 0 0 0 0 0 ] for key[pose]"); + } + { + // clear the contents of the buffer + buffer.str(""); + std::string path = pathBase +"/bad_syntax_double.sdf"; + sdf::SDFPtr sdf(new sdf::SDF()); + sdf::init(sdf); + + sdf::readFile(path, sdf); + EXPECT_PRED2(contains, buffer.str(), + "Unable to set value [bad ] for key[linear]"); + } + { + // clear the contents of the buffer + buffer.str(""); + std::string path = pathBase +"/bad_syntax_vector.sdf"; + sdf::SDFPtr sdf(new sdf::SDF()); + sdf::init(sdf); + + sdf::readFile(path, sdf); + EXPECT_PRED2(contains, buffer.str(), + "Unable to set value [0 1 bad ] for key[gravity]"); + } + + // Revert cerr rdbug so as to not interfere with other tests + std::cerr.rdbuf(old); +#ifdef _WIN32 + sdf::Console::Instance()->SetQuiet(true); +#endif +} + +///////////////////////////////////////////////// ///////////////////////////////////////////////// /// Main int main(int argc, char **argv) diff --git a/test/sdf/bad_syntax_double.sdf b/test/sdf/bad_syntax_double.sdf new file mode 100644 index 000000000..7f6c7c2e3 --- /dev/null +++ b/test/sdf/bad_syntax_double.sdf @@ -0,0 +1,13 @@ + + + + + + + bad + + + + + + diff --git a/test/sdf/bad_syntax_pose.sdf b/test/sdf/bad_syntax_pose.sdf new file mode 100644 index 000000000..7c10750ad --- /dev/null +++ b/test/sdf/bad_syntax_pose.sdf @@ -0,0 +1,10 @@ + + + + + bad 0 0 0 0 0 + + + + + diff --git a/test/sdf/bad_syntax_vector.sdf b/test/sdf/bad_syntax_vector.sdf new file mode 100644 index 000000000..b454d4403 --- /dev/null +++ b/test/sdf/bad_syntax_vector.sdf @@ -0,0 +1,7 @@ + + + + 0 1 bad + + + diff --git a/test/sdf/box_bad_test.world b/test/sdf/box_bad_test.world index 7516a9d13..5a827b32e 100644 --- a/test/sdf/box_bad_test.world +++ b/test/sdf/box_bad_test.world @@ -2,7 +2,7 @@ - 0 0.5 0.0 0.0 0.0 + 0 0.5 0.0 0.0 0.0 0.0 0.05