From a70801957a52a65dd8935a4b33270cb7959402d6 Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Tue, 23 Aug 2016 13:17:38 +0200 Subject: [PATCH] [4483] Review comments addressed - Implemented new test for checking integer boundaries - checkTokenInteger documented - indentation fixed in parser.yy - integer check in lexer.ll extended from int to int64_t - a bug fixed in EvalContext::convertUint32 - User's Guide now explicitly mentions network order --- doc/guide/classify.xml | 4 +++- src/lib/eval/eval_context.cc | 2 +- src/lib/eval/lexer.ll | 6 ++++- src/lib/eval/parser.yy | 12 +++++----- src/lib/eval/tests/context_unittest.cc | 4 ++++ src/lib/eval/tests/evaluate_unittest.cc | 31 ++++++++++++++++++++++++- 6 files changed, 49 insertions(+), 10 deletions(-) diff --git a/doc/guide/classify.xml b/doc/guide/classify.xml index 7876aed059..765daf71d2 100644 --- a/doc/guide/classify.xml +++ b/doc/guide/classify.xml @@ -433,7 +433,9 @@ it is easier to use decimal notation to represent integers, but it is also possible to use hex notation. When using hex notation to represent an integer care should be taken to make sure the value is represented as 32 - bits, e.g. use 0x00000001 instead of 0x1 or 0x01. + bits, e.g. use 0x00000001 instead of 0x1 or 0x01. Also, make + sure the value is specified in network order, e.g. 1 is + represented as 0x00000001. diff --git a/src/lib/eval/eval_context.cc b/src/lib/eval/eval_context.cc index ff1a0c8609..8cd13653b5 100644 --- a/src/lib/eval/eval_context.cc +++ b/src/lib/eval/eval_context.cc @@ -139,7 +139,7 @@ EvalContext::convertUint32(const std::string& number, } catch (const boost::bad_lexical_cast &) { error(loc, "Invalid value in " + number); } - if (n >= std::numeric_limits::max()) { + if (n > std::numeric_limits::max()) { error(loc, "Invalid value in " + number + ". Allowed range: 0..4294967295"); } diff --git a/src/lib/eval/lexer.ll b/src/lib/eval/lexer.ll index fdefcecd63..f7bd53593a 100644 --- a/src/lib/eval/lexer.ll +++ b/src/lib/eval/lexer.ll @@ -110,7 +110,11 @@ addr6 [0-9a-fA-F]*\:[0-9a-fA-F]*\:[0-9a-fA-F:.]* std::string tmp(yytext); try { - static_cast(boost::lexical_cast(tmp)); + // In substring we want to use negative values (e.g. -1). + // In enterprise-id we need to use values up to 0xffffffff. + // To cover both of those use cases, we need at least + // int64_t. + static_cast(boost::lexical_cast(tmp)); } catch (const boost::bad_lexical_cast &) { driver.error(loc, "Failed to convert " + tmp + " to an integer."); } diff --git a/src/lib/eval/parser.yy b/src/lib/eval/parser.yy index aad6c8e88d..5bfadcfed0 100644 --- a/src/lib/eval/parser.yy +++ b/src/lib/eval/parser.yy @@ -371,18 +371,18 @@ string_expr : STRING TokenVendor::DATA, index)); ctx.expression.push_back(vendor_class); } - | integer_expr + | integer_expr { TokenPtr integer(new TokenInteger($1)); ctx.expression.push_back(integer); } ; -integer_expr: INTEGER - { - $$ = ctx.convertUint32($1, @1); - } - ; +integer_expr : INTEGER + { + $$ = ctx.convertUint32($1, @1); + } + ; option_code : INTEGER { diff --git a/src/lib/eval/tests/context_unittest.cc b/src/lib/eval/tests/context_unittest.cc index 6272c9d88f..25b02b47f7 100644 --- a/src/lib/eval/tests/context_unittest.cc +++ b/src/lib/eval/tests/context_unittest.cc @@ -96,6 +96,10 @@ public: EXPECT_TRUE(eq); } + /// @brief Checks if the given token is integer with expected value + /// + /// @param token token to be inspected + /// @param exp_value expected integer value of the token void checkTokenInteger(const TokenPtr& token, uint32_t exp_value) { ASSERT_TRUE(token); boost::shared_ptr integer = diff --git a/src/lib/eval/tests/evaluate_unittest.cc b/src/lib/eval/tests/evaluate_unittest.cc index d27fd652b6..8e40547972 100644 --- a/src/lib/eval/tests/evaluate_unittest.cc +++ b/src/lib/eval/tests/evaluate_unittest.cc @@ -314,7 +314,8 @@ public: bool result; bool parsed; - EXPECT_NO_THROW(parsed = eval.parseString(expr)); + EXPECT_NO_THROW(parsed = eval.parseString(expr)) + << " while parsing expression " << expr; EXPECT_TRUE(parsed) << " for expression " << expr; switch (u) { @@ -330,6 +331,19 @@ public: EXPECT_EQ(exp_result, result) << " for expression " << expr; } + + /// @brief Checks that specified expression throws expected exception. + /// + /// @tparam ex exception type expected to be thrown + /// @param expr expression to be evaluated + template + void testExpressionNegative(const std::string& expr, + const Option::Universe& u = Option::V4) { + EvalContext eval(u); + + EXPECT_THROW(eval.parseString(expr), ex) << "while parsing expression " + << expr; + } }; // This is a quick way to check if certain expressions are valid or not and @@ -402,6 +416,8 @@ TEST_F(ExpressionsTest, expressionsPkt4Hlen) { testExpression(Option::V4, "pkt4.mac == 0x010203040506", true); } +// Test if expressions message type can be detected in Pkt4. +// It also doubles as a check for integer comparison here. TEST_F(ExpressionsTest, expressionsPkt4type) { // We can inspect the option content directly, but @@ -416,4 +432,17 @@ TEST_F(ExpressionsTest, expressionsPkt4type) { testExpression(Option::V4, "pkt4.msgtype == 2", false); } +// This tests if inappropriate values (negative, too large) are +// rejected, but extremal value still allowed for uint32_t are ok. +TEST_F(ExpressionsTest, invalidIntegers) { + + // These are the extremal uint32_t values that still should be accepted. + testExpression(Option::V4, "4294967295 == 0", false); + + // Negative integers should be rejected. + testExpressionNegative("4294967295 == -1"); + + // Oops, one too much. + testExpressionNegative("4294967296 == 0"); +} }; -- 2.47.2