From 2da57bd1576c99ff4305cab7b6ad8c1a39ac2fa7 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Wed, 6 Jun 2018 11:26:08 -0400 Subject: [core] Fix crash due to mixing legacy filters and expressions (#12065) * Fix crash due to mixing legacy filters and expressions In some cases, (invalid) nested filters that used a mix of legacy filter syntax and expression syntax caused a crash due to a failure to propagate parsing errors from deeper within the filter expression. These errors went undetected in part because these conversion functions returned unique_ptr values (or vectors thereof), using {nullptr} to represent a parsing error, but the core expression classes expect unique_ptr that are never null. This changes over to using expression::ParseResult (aka optional>), to represent conversion failure the same way we do in the rest of the expression system. * Fix clang 3.8 / gcc 4.9 issue --- src/mbgl/style/conversion/filter.cpp | 84 ++++++++++++++++++++++-------------- test/style/filter.test.cpp | 12 ++++++ 2 files changed, 63 insertions(+), 33 deletions(-) diff --git a/src/mbgl/style/conversion/filter.cpp b/src/mbgl/style/conversion/filter.cpp index 386d85e921..ad13470bca 100644 --- a/src/mbgl/style/conversion/filter.cpp +++ b/src/mbgl/style/conversion/filter.cpp @@ -13,7 +13,7 @@ namespace conversion { using namespace mbgl::style::expression; static bool isExpression(const Convertible& filter); -std::unique_ptr convertLegacyFilter(const Convertible& values, Error& error); +ParseResult convertLegacyFilter(const Convertible& values, Error& error); optional Converter::operator()(const Convertible& value, Error& error) const { if (isExpression(value)) { @@ -26,9 +26,12 @@ optional Converter::operator()(const Convertible& value, Error& return { Filter(std::move(parseResult)) }; } } else { - std::unique_ptr expression = convertLegacyFilter(value, error); - if (!expression) return {}; - return Filter(optional>(std::move(expression))); + ParseResult expression = convertLegacyFilter(value, error); + if (!expression) { + assert(error.message.size() > 0); + return {}; + } + return Filter(optional>(std::move(*expression))); } } @@ -68,51 +71,62 @@ bool isExpression(const Convertible& filter) { } } -std::unique_ptr createExpression(std::string op, std::vector> args, Error& error) { +ParseResult createExpression(std::string op, optional>> args, Error& error) { + if (!args) return {}; + assert(std::all_of(args->begin(), args->end(), [](const std::unique_ptr &e) { + return bool(e.get()); + })); + if (op == "any") { - return std::make_unique(std::move(args)); - + return {std::make_unique(std::move(*args))}; } else if (op == "all") { - return std::make_unique(std::move(args)); - + return {std::make_unique(std::move(*args))}; } else { ParsingContext parsingContext(type::Boolean); - ParseResult parseResult = createCompoundExpression(op, std::move(args), parsingContext); + ParseResult parseResult = createCompoundExpression(op, std::move(*args), parsingContext); if (!parseResult) { error = { parsingContext.getCombinedErrors() }; return {}; } else { - return std::move(*parseResult); + return parseResult; } } } -std::unique_ptr createExpression(std::string op, std::unique_ptr expression, Error& error) { +ParseResult createExpression(std::string op, ParseResult arg, Error& error) { + if (!arg) { + return {}; + } + std::vector> args; - args.push_back(std::move(expression)); + args.push_back(std::move(*arg)); return createExpression(op, std::move(args), error); } -std::unique_ptr convertLiteral(const Convertible& convertible, Error& error) { +ParseResult convertLiteral(const Convertible& convertible, Error& error) { ParsingContext parsingContext; ParseResult parseResult = Literal::parse(convertible, parsingContext); if (parseResult) { - return std::move(*parseResult); + return parseResult; } else { error = { parsingContext.getCombinedErrors() }; return {}; } } -std::vector> convertLiteralArray(const Convertible &input, Error& error, std::size_t startIndex = 0) { +optional>> convertLiteralArray(const Convertible &input, Error& error, std::size_t startIndex = 0) { std::vector> output; for (std::size_t i = startIndex; i < arrayLength(input); i++) { - output.push_back(convertLiteral(arrayMember(input, i), error)); + ParseResult literal = convertLiteral(arrayMember(input, i), error); + if (!literal) { + return {}; + } + output.push_back(std::move(*literal)); } - return output; + return {std::move(output)}; } -std::unique_ptr convertLegacyComparisonFilter(const Convertible& values, Error& error, optional opOverride = {}) { +ParseResult convertLegacyComparisonFilter(const Convertible& values, Error& error, optional opOverride = {}) { optional op = opOverride ? opOverride : toString(arrayMember(values, 0)); optional property = toString(arrayMember(values, 1)); @@ -128,29 +142,29 @@ std::unique_ptr convertLegacyComparisonFilter(const Convertible& val } } -std::unique_ptr convertLegacyHasFilter(const Convertible& values, Error& error) { +ParseResult convertLegacyHasFilter(const Convertible& values, Error& error) { optional property = toString(arrayMember(values, 1)); if (!property) { error = { "filter property must be a string" }; return {}; } else if (*property == "$type") { - return std::make_unique(true); + return {std::make_unique(true)}; } else if (*property == "$id") { return createExpression("filter-has-id", std::vector>(), error); } else { - return createExpression("filter-has", std::make_unique(*property), error); + return createExpression("filter-has", {std::make_unique(*property)}, error); } } -std::unique_ptr convertLegacyInFilter(const Convertible& values, Error& error) { +ParseResult convertLegacyInFilter(const Convertible& values, Error& error) { optional property = toString(arrayMember(values, 1)); if (!property) { error = { "filter property must be a string" }; return {}; } else if (arrayLength(values) == 0) { - return std::make_unique(false); + return {std::make_unique(false)}; } else if (*property == "$type") { return createExpression("filter-type-in", convertLiteralArray(values, error, 2), error); } else if (*property == "$id") { @@ -160,17 +174,21 @@ std::unique_ptr convertLegacyInFilter(const Convertible& values, Err } } -std::vector> convertLegacyFilterArray(const Convertible &input, Error& error, std::size_t startIndex = 0) { +optional>> convertLegacyFilterArray(const Convertible &input, Error& error, std::size_t startIndex = 0) { std::vector> output; for (std::size_t i = startIndex; i < arrayLength(input); i++) { - output.push_back(convertLegacyFilter(arrayMember(input, i), error)); + optional> child = convertLegacyFilter(arrayMember(input, i), error); + if (!child) { + return {}; + } + output.push_back(std::move(*child)); } - return output; + return {std::move(output)}; } -std::unique_ptr convertLegacyFilter(const Convertible& values, Error& error) { +ParseResult convertLegacyFilter(const Convertible& values, Error& error) { if (isUndefined(values)) { - return std::make_unique(true); + return {std::make_unique(true)}; } optional op = toString(arrayMember(values, 0)); @@ -179,9 +197,9 @@ std::unique_ptr convertLegacyFilter(const Convertible& values, Error error = { "filter operator must be a string" }; return {}; } else if (arrayLength(values) <= 1) { - return std::make_unique(*op != "any"); + return {std::make_unique(*op != "any")}; } else { - return ( + return { *op == "==" || *op == "<" || *op == ">" || @@ -195,8 +213,8 @@ std::unique_ptr convertLegacyFilter(const Convertible& values, Error *op == "!in" ? createExpression("!", convertLegacyInFilter(values, error), error) : *op == "has" ? convertLegacyHasFilter(values, error) : *op == "!has" ? createExpression("!", convertLegacyHasFilter(values, error), error) : - std::make_unique(true) - ); + ParseResult(std::make_unique(true)) + }; } } diff --git a/test/style/filter.test.cpp b/test/style/filter.test.cpp index c59a73eab1..c8f1d3b808 100644 --- a/test/style/filter.test.cpp +++ b/test/style/filter.test.cpp @@ -3,6 +3,11 @@ #include #include +#include +#include +#include +#include + #include #include #include @@ -176,6 +181,13 @@ TEST(Filter, LegacyProperty) { ASSERT_FALSE(filter("[\"==\", \"two\", 4]", {{"two", std::string("2")}})); } +TEST(Filter, ExpressionLegacyMix) { + conversion::Error error; + optional filter = conversion::convertJSON(R"(["any", ["all", ["==", ["geometry-type"], "LineString"]], ["==", "x", 1]])", error); + EXPECT_FALSE(bool(filter)); + EXPECT_TRUE(error.message.size() > 0); +} + TEST(Filter, ZoomExpressionNested) { ASSERT_TRUE(filter(R"(["==", ["get", "two"], ["zoom"]])", {{"two", int64_t(2)}}, {}, FeatureType::Point, {}, 2.0f)); ASSERT_FALSE(filter(R"(["==", ["get", "two"], ["+", ["zoom"], 1]])", {{"two", int64_t(2)}}, {}, FeatureType::Point, {}, 2.0f)); -- cgit v1.2.1