tei/MapLibreParticleLab
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[core] Fix crash due to mixing legacy filters and expressions (#12065)

Browse Source 
* 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<Expression> values (or vectors thereof), using
{nullptr} to represent a parsing error, but the core expression classes
expect unique_ptr<Expression> that are never null.

This changes over to using expression::ParseResult (aka
optional<unique_ptr<Expression>>), to represent conversion failure
the same way we do in the rest of the expression system.

* Fix clang 3.8 / gcc 4.9 issue
This commit is contained in:
Anand Thakker
2018-06-06 11:26:08 -04:00
committed by GitHub
parent 6eee872955
commit 2da57bd157
2 changed files with 63 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

84
src/mbgl/style/conversion/filter.cpp
View File

@@ -13,7 +13,7 @@ namespace conversion {
using namespace mbgl::style::expression;
static bool isExpression(const Convertible& filter);
std::unique_ptr<Expression> convertLegacyFilter(const Convertible& values, Error& error);
ParseResult convertLegacyFilter(const Convertible& values, Error& error);
optional<Filter> Converter<Filter>::operator()(const Convertible& value, Error& error) const {
if (isExpression(value)) {
@@ -26,9 +26,12 @@ optional<Filter> Converter<Filter>::operator()(const Convertible& value, Error&
return { Filter(std::move(parseResult)) };
}
} else {
std::unique_ptr<Expression> expression = convertLegacyFilter(value, error);
if (!expression) return {};
return Filter(optional<std::unique_ptr<Expression>>(std::move(expression)));
ParseResult expression = convertLegacyFilter(value, error);
if (!expression) {
assert(error.message.size() > 0);
return {};
}
return Filter(optional<std::unique_ptr<Expression>>(std::move(*expression)));
}
}
@@ -68,51 +71,62 @@ bool isExpression(const Convertible& filter) {
}
}
std::unique_ptr<Expression> createExpression(std::string op, std::vector<std::unique_ptr<Expression>> args, Error& error) {
ParseResult createExpression(std::string op, optional<std::vector<std::unique_ptr<Expression>>> args, Error& error) {
if (!args) return {};
assert(std::all_of(args->begin(), args->end(), [](const std::unique_ptr<Expression> &e) {
return bool(e.get());
}));
if (op == "any") {
return std::make_unique<Any>(std::move(args));
return {std::make_unique<Any>(std::move(*args))};
} else if (op == "all") {
return std::make_unique<All>(std::move(args));
return {std::make_unique<All>(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<Expression> createExpression(std::string op, std::unique_ptr<Expression> expression, Error& error) {
ParseResult createExpression(std::string op, ParseResult arg, Error& error) {
if (!arg) {
return {};
}
std::vector<std::unique_ptr<Expression>> args;
args.push_back(std::move(expression));
args.push_back(std::move(*arg));
return createExpression(op, std::move(args), error);
}
std::unique_ptr<Expression> 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<std::unique_ptr<Expression>> convertLiteralArray(const Convertible &input, Error& error, std::size_t startIndex = 0) {
optional<std::vector<std::unique_ptr<Expression>>> convertLiteralArray(const Convertible &input, Error& error, std::size_t startIndex = 0) {
std::vector<std::unique_ptr<Expression>> 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<Expression> convertLegacyComparisonFilter(const Convertible& values, Error& error, optional<std::string> opOverride = {}) {
ParseResult convertLegacyComparisonFilter(const Convertible& values, Error& error, optional<std::string> opOverride = {}) {
optional<std::string> op = opOverride ? opOverride : toString(arrayMember(values, 0));
optional<std::string> property = toString(arrayMember(values, 1));
@@ -128,29 +142,29 @@ std::unique_ptr<Expression> convertLegacyComparisonFilter(const Convertible& val
}
}
std::unique_ptr<Expression> convertLegacyHasFilter(const Convertible& values, Error& error) {
ParseResult convertLegacyHasFilter(const Convertible& values, Error& error) {
optional<std::string> property = toString(arrayMember(values, 1));
if (!property) {
error = { "filter property must be a string" };
return {};
} else if (*property == "$type") {
return std::make_unique<Literal>(true);
return {std::make_unique<Literal>(true)};
} else if (*property == "$id") {
return createExpression("filter-has-id", std::vector<std::unique_ptr<Expression>>(), error);
} else {
return createExpression("filter-has", std::make_unique<Literal>(*property), error);
return createExpression("filter-has", {std::make_unique<Literal>(*property)}, error);
}
}
std::unique_ptr<Expression> convertLegacyInFilter(const Convertible& values, Error& error) {
ParseResult convertLegacyInFilter(const Convertible& values, Error& error) {
optional<std::string> property = toString(arrayMember(values, 1));
if (!property) {
error = { "filter property must be a string" };
return {};
} else if (arrayLength(values) == 0) {
return std::make_unique<Literal>(false);
return {std::make_unique<Literal>(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<Expression> convertLegacyInFilter(const Convertible& values, Err
}
}
std::vector<std::unique_ptr<Expression>> convertLegacyFilterArray(const Convertible &input, Error& error, std::size_t startIndex = 0) {
optional<std::vector<std::unique_ptr<Expression>>> convertLegacyFilterArray(const Convertible &input, Error& error, std::size_t startIndex = 0) {
std::vector<std::unique_ptr<Expression>> output;
for (std::size_t i = startIndex; i < arrayLength(input); i++) {
output.push_back(convertLegacyFilter(arrayMember(input, i), error));
optional<std::unique_ptr<Expression>> child = convertLegacyFilter(arrayMember(input, i), error);
if (!child) {
return {};
}
output.push_back(std::move(*child));
}
return output;
return {std::move(output)};
}
std::unique_ptr<Expression> convertLegacyFilter(const Convertible& values, Error& error) {
ParseResult convertLegacyFilter(const Convertible& values, Error& error) {
if (isUndefined(values)) {
return std::make_unique<Literal>(true);
return {std::make_unique<Literal>(true)};
}
optional<std::string> op = toString(arrayMember(values, 0));
@@ -179,9 +197,9 @@ std::unique_ptr<Expression> convertLegacyFilter(const Convertible& values, Error
error = { "filter operator must be a string" };
return {};
} else if (arrayLength(values) <= 1) {
return std::make_unique<Literal>(*op != "any");
return {std::make_unique<Literal>(*op != "any")};
} else {
return (
return {
*op == "==" ||
*op == "<" ||
*op == ">" ||
@@ -195,8 +213,8 @@ std::unique_ptr<Expression> 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<Literal>(true)
);
ParseResult(std::make_unique<Literal>(true))
};
}
}

12
test/style/filter.test.cpp
View File

@@ -3,6 +3,11 @@
#include <mbgl/util/geometry.hpp>
#include <mbgl/test/stub_geometry_tile_feature.hpp>
#include <mbgl/util/rapidjson.hpp>
#include <rapidjson/writer.h>
#include <rapidjson/stringbuffer.h>
#include <mbgl/style/conversion/stringify.hpp>
#include <mbgl/style/filter.hpp>
#include <mbgl/style/conversion/json.hpp>
#include <mbgl/style/conversion/filter.hpp>
@@ -176,6 +181,13 @@ TEST(Filter, LegacyProperty) {
ASSERT_FALSE(filter("[\"==\", \"two\", 4]", {{"two", std::string("2")}}));
}
TEST(Filter, ExpressionLegacyMix) {
conversion::Error error;
optional<Filter> filter = conversion::convertJSON<Filter>(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));