Skip to content

Commit

Permalink
Fix[bmqeval]: limit expression length to avoid stack overflow (bloomb…
Browse files Browse the repository at this point in the history 
…erg#441)

Signed-off-by: Evgeny Malygin <[email protected]>
  • Loading branch information
@678098 @alexander-e1off
678098 authored and alexander-e1off committed Oct 24, 2024
1 parent c44dc8b commit cad3c2c
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 66 deletions.
22 changes: 14 additions & 8 deletions src/groups/bmq/bmqeval/bmqeval_simpleevaluator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ void SimpleEvaluator::parse(const bsl::string& expression,
context.d_numProperties = 0;
context.d_os.reset();

if (expression.length() > k_MAX_EXPRESSION_LENGTH) {
context.d_os << "expression is too long (" << expression.length()
<< "), max allowed length: " << k_MAX_EXPRESSION_LENGTH;
context.d_lastError = ErrorType::e_TOO_LONG;
return; // RETURN
}

bsl::istringstream is(expression, context.d_allocator);
SimpleEvaluatorScanner scanner;
scanner.switch_streams(&is, &context.d_os);
Expand All @@ -90,28 +97,27 @@ void SimpleEvaluator::parse(const bsl::string& expression,

if (parser.parse() != 0) {
context.d_lastError = ErrorType::e_SYNTAX;

return; // RETURN
}

if (context.d_numOperators > k_MAX_OPERATORS) {
context.d_os << "too many operators";
context.d_os << "too many operators (" << context.d_numOperators
<< "), max allowed operators: " << k_MAX_OPERATORS;
context.d_lastError = ErrorType::e_TOO_COMPLEX;

return; // RETURN
}

if (context.d_numProperties == 0) {
context.d_os << "expression does not use any properties";
context.d_lastError = ErrorType::e_NO_PROPERTIES;

return; // RETURN
}

if (context.d_numProperties > k_MAX_PROPERTIES) {
context.d_os << "expression uses too many properties";
context.d_os << "expression uses too many properties ("
<< context.d_numProperties
<< "), max allowed properties: " << k_MAX_PROPERTIES;
context.d_lastError = ErrorType::e_TOO_COMPLEX;

return; // RETURN
}
}
Expand Down Expand Up @@ -204,8 +210,8 @@ SimpleEvaluator::IntegerLiteral::evaluate(EvaluationContext& context) const
// class SimpleEvaluator::BooleanLiteral
// -------------------------------------

bdld::Datum
SimpleEvaluator::BooleanLiteral::evaluate(EvaluationContext& context) const
bdld::Datum SimpleEvaluator::BooleanLiteral::evaluate(
BSLS_ANNOTATION_UNUSED EvaluationContext& context) const
{
return bdld::Datum::createBoolean(d_value);
}
Expand Down
71 changes: 50 additions & 21 deletions src/groups/bmq/bmqeval/bmqeval_simpleevaluator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,25 @@ class EvaluationContext;

struct ErrorType {
enum Enum {
e_OK = 0 // no error
,
/// no error
e_OK = 0,

e_COMPILATION_FIRST = -100,
e_SYNTAX = -100 // syntax error
,
e_NO_PROPERTIES = -101 // expression does not use any property
,
e_TOO_COMPLEX = -102 // too many properties
// or operators in expression
,
e_COMPILATION_LAST = -102,

/// syntax error
e_SYNTAX = -100,

/// expression does not use any property
e_NO_PROPERTIES = -101,

/// too many properties or operators in expression
e_TOO_COMPLEX = -102,

/// expression string is too long
e_TOO_LONG = -103,

e_COMPILATION_LAST = -103,

e_EVALUATION_FIRST = -1,
e_NAME = -1,
e_TYPE = -2,
Expand Down Expand Up @@ -492,10 +500,14 @@ class SimpleEvaluator {
public:
// PUBLIC CONSTANTS
enum {
/// The maximum length of an expression string.
k_MAX_EXPRESSION_LENGTH = 128,

/// The maximum number of operators allowed in a single expression.
k_MAX_OPERATORS = 10,
k_MAX_OPERATORS = 10,

/// The maximum number of properties allowed in a single expression.
k_MAX_PROPERTIES = 10
// The maximum number of properties allowed in a single expression.
};

// CREATORS
Expand Down Expand Up @@ -706,19 +718,36 @@ class EvaluationContext {
inline const char* ErrorType::toString(ErrorType::Enum value)
{
switch (value) {
case 0: return ""; // RETURN
case bmqeval::ErrorType::e_SYNTAX: return "syntax error"; // RETURN
case bmqeval::ErrorType::e_TOO_COMPLEX:
return "subscription expression is too complex"; // RETURN
case bmqeval::ErrorType::e_NO_PROPERTIES:
case bmqeval::ErrorType::e_OK: {
return ""; // RETURN
}
case bmqeval::ErrorType::e_SYNTAX: {
return "syntax error"; // RETURN
}
case bmqeval::ErrorType::e_NO_PROPERTIES: {
return "expression does not use any property"; // RETURN
case bmqeval::ErrorType::e_NAME:
}
case bmqeval::ErrorType::e_TOO_COMPLEX: {
return "subscription expression is too complex"; // RETURN
}
case bmqeval::ErrorType::e_TOO_LONG: {
return "subscription expression is too long"; // RETURN
}
case bmqeval::ErrorType::e_NAME: {
return "undefined name in subscription expression"; // RETURN
case bmqeval::ErrorType::e_TYPE:
}
case bmqeval::ErrorType::e_TYPE: {
return "type error in expression"; // RETURN
case bmqeval::ErrorType::e_BINARY:
}
case bmqeval::ErrorType::e_BINARY: {
return "binary properties are not supported"; // RETURN
default: return "unknown error"; // RETURN
}
case bmqeval::ErrorType::e_UNDEFINED: {
return "undefined error"; // RETURN
}
default: {
return "unknown error"; // RETURN
}
}
}

Expand Down
92 changes: 56 additions & 36 deletions src/groups/bmq/bmqeval/bmqeval_simpleevaluator.t.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ class MockPropertiesReader : public PropertiesReader {
// MANIPULATORS

/// Return a `bdld::Datum` object with value for the specified `name`.
/// Use the specified `allocator` for any memory allocation.
virtual bdld::Datum get(const bsl::string& name,
bslma::Allocator* allocator)
/// The `bslma::Allocator*` argument is unused.
bdld::Datum get(const bsl::string& name,
bslma::Allocator*) BSLS_KEYWORD_OVERRIDE
{
bsl::unordered_map<bsl::string, bdld::Datum>::const_iterator iter =
d_map.find(name);
Expand Down Expand Up @@ -124,6 +124,30 @@ static bsl::string makeTooManyOperators()
return os.str();
}

static bsl::string makeTooLongExpression()
{
mwcu::MemOutStream os(s_allocator_p);

// Note that we want to create `k_STACK_SIZE` nested NOT objects in AST,
// and when we call destructor chain for all these objects, we'll need
// much more memory on a stack, since each destructor address is not a
// single byte, and also we need to call shared_ptr and Not destructors in
// pairs. But the initial guess of `k_STACK_SIZE` is more than sufficient
// to cause segfault if this case is not handled properly.
const size_t k_STACK_SIZE = 1024 * 1024;
ASSERT(SimpleEvaluator::k_MAX_EXPRESSION_LENGTH < k_STACK_SIZE);

for (size_t i = 0; i < k_STACK_SIZE; i += 16) {
// Combining `!` and `~` differently in case we want to introduce
// "NOT" optimization one day (remove double sequential "NOTs")
os << "!!!!~~~~!~!~!!~~";
}

os << "x";

return os.str();
}

static void test1_compilationErrors()
{
struct TestParameters {
Expand All @@ -139,10 +163,13 @@ static void test1_compilationErrors()
"syntax error, unexpected invalid character at offset 3"},
{makeTooManyOperators(),
ErrorType::e_TOO_COMPLEX,
"too many operators"},
"too many operators (12), max allowed operators: 10"},
{"true && true",
ErrorType::e_NO_PROPERTIES,
"expression does not use any properties"},
{makeTooLongExpression(),
ErrorType::e_TOO_LONG,
"expression is too long (1048577), max allowed length: 128"},

// only C-style operator variants supported
{"val = 42",
Expand All @@ -157,6 +184,27 @@ static void test1_compilationErrors()
{"val <> 42",
ErrorType::e_SYNTAX,
"syntax error, unexpected > at offset 5"},

// unsupported_ints
{"i_0 != 9223372036854775808",
ErrorType::e_SYNTAX,
"integer overflow at offset 7"}, // 2 ** 63
{"i_0 != -170141183460469231731687303715884105728",
ErrorType::e_SYNTAX,
"integer overflow at offset 7"}, // -(2 ** 127)
{"i_0 != 170141183460469231731687303715884105727",
ErrorType::e_SYNTAX,
"integer overflow at offset 7"}, // 2 ** 127 - 1
{"i_0 != "
"-5789604461865809771178549250434395392663499233282028201972879200395"
"6564819968",
ErrorType::e_SYNTAX,
"integer overflow at offset 7"}, // -(2 ** 255)
{"i_0 != "
"57896044618658097711785492504343953926634992332820282019728792003956"
"564819967",
ErrorType::e_SYNTAX,
"integer overflow at offset 7"}, // 2 ** 255 - 1
};
const TestParameters* testParametersEnd = testParameters +
sizeof(testParameters) /
Expand Down Expand Up @@ -287,7 +335,6 @@ static void test3_evaluation()
struct TestParameters {
const char* expression;
bool expected;
bool expectCompilationFailure;
} testParameters[] = {
{"b_true", true},

Expand Down Expand Up @@ -427,25 +474,6 @@ static void test3_evaluation()
{"i_0 != -9223372036854775807", true}, // -(2 ** 63) + 1
{"i_0 != 9223372036854775807", true}, // 2 ** 63 - 1
{"i_0 != -9223372036854775808", true}, // -(2 ** 63)

// unsupported_ints
{"i_0 != 9223372036854775808", false, true}, // 2 ** 63
{"i_0 != -170141183460469231731687303715884105728",
false,
true}, // -(2 ** 127)
{"i_0 != 170141183460469231731687303715884105727",
false,
true}, // 2 ** 127 - 1
{"i_0 != "
"-5789604461865809771178549250434395392663499233282028201972879200395"
"6564819968",
false,
true}, // -(2 ** 255)
{"i_0 != "
"57896044618658097711785492504343953926634992332820282019728792003956"
"564819967",
false,
true}, // 2 ** 255 - 1
};
const TestParameters* testParametersEnd = testParameters +
sizeof(testParameters) /
Expand All @@ -461,20 +489,12 @@ static void test3_evaluation()

ASSERT(!evaluator.isValid());

if (int rc = evaluator.compile(parameters->expression,
compilationContext)) {
if (!parameters->expectCompilationFailure) {
PV(bsl::string("UNEXPECTED: ") +
compilationContext.lastErrorMessage());
ASSERT(false);
}
if (evaluator.compile(parameters->expression, compilationContext)) {
PV(bsl::string("UNEXPECTED: ") +
compilationContext.lastErrorMessage());
ASSERT(false);
}
else {
if (parameters->expectCompilationFailure) {
PV("Expected compilation failure");
ASSERT(false);
}

ASSERT(evaluator.isValid());
ASSERT_EQ(evaluator.evaluate(evaluationContext),
parameters->expected);
Expand Down
2 changes: 1 addition & 1 deletion src/groups/bmq/bmqeval/bmqeval_simpleevaluatorparser.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ expression
{ $$ = ctx.makeLiteral<SimpleEvaluator::IntegerLiteral>($1); }
| OVERFLOW
{
ctx.d_os << " integer overflow at offset " << scanner.lastTokenLocation();
ctx.d_os << "integer overflow at offset " << scanner.lastTokenLocation();
YYABORT;
}
| STRING
Expand Down

0 comments on commit cad3c2c

Please sign in to comment.