Fix buffer overflow in argument parsing caused by lexer returning len…

…gth beyond length of string (#979) * Test that lexer never returns length longer than string Signed-off-by: Shane Loretz <[email protected]> * Fix bug where lexer returned length longer than string Signed-off-by: Shane Loretz <[email protected]> * Test that peeking 2 ahead never goes beyond NONE or EOF Signed-off-by: Shane Loretz <[email protected]> * Stop peeking if the first lexeme is NONE or EOF Signed-off-by: Shane Loretz <[email protected]>
ros2 · Apr 18, 2022 · 392f0d3 · 392f0d3
1 parent 71baed4
commit 392f0d3
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 4 deletions.
diff --git a/rcl/include/rcl/lexer.h b/rcl/include/rcl/lexer.h
@@ -88,6 +88,7 @@ typedef enum rcl_lexeme_e
  * This function analyzes a string to see if it starts with a valid lexeme.
  * If the string does not begin with a valid lexeme then lexeme will be RCL_LEXEME_NONE, and the
  * length will be set to include the character that made it impossible.
+ * It will never be longer than the length of the string.
  * If the first character is '\0' then lexeme will be RCL_LEXEME_EOF.
  *
  * <hr>

diff --git a/rcl/src/rcl/lexer.c b/rcl/src/rcl/lexer.c
@@ -650,10 +650,11 @@ rcl_lexer_analyze(
       movement = state->else_movement;
     }
 
-    // Move the lexer to another character in the string
     if (0u == movement) {
-      // Go forwards 1 char
-      ++(*length);
+      if ('\0' != current_char) {
+        // Go forwards 1 char as long as the end hasn't been reached
+        ++(*length);
+      }
     } else {
       // Go backwards N chars
       if (movement - 1u > *length) {

diff --git a/rcl/src/rcl/lexer_lookahead.c b/rcl/src/rcl/lexer_lookahead.c
@@ -141,6 +141,12 @@ rcl_lexer_lookahead2_peek2(
   }
   RCL_CHECK_ARGUMENT_FOR_NULL(next_type2, RCL_RET_INVALID_ARGUMENT);
 
+  if (RCL_LEXEME_NONE == *next_type1 || RCL_LEXEME_EOF == *next_type1) {
+    // No need to peek further
+    *next_type2 = *next_type1;
+    return ret;
+  }
+
   size_t length;
 
   if (buffer->impl->text_idx >= buffer->impl->end[1]) {

diff --git a/rcl/test/rcl/test_lexer.cpp b/rcl/test/rcl/test_lexer.cpp
@@ -45,7 +45,8 @@ class CLASSNAME (TestLexerFixture, RMW_IMPLEMENTATION) : public ::testing::Test
     rcl_ret_t ret = rcl_lexer_analyze(text, &actual_lexeme, &length); \
     ASSERT_EQ(RCL_RET_OK, ret); \
     EXPECT_EQ(expected_lexeme, actual_lexeme); \
-    std::string actual_text(text, length); \
+    std::string actual_text(text, 0u, length); \
+    EXPECT_EQ(length, actual_text.size()); \
     EXPECT_STREQ(expected_text, actual_text.c_str()); \
   } while (false)
 

diff --git a/rcl/test/rcl/test_lexer_lookahead.cpp b/rcl/test/rcl/test_lexer_lookahead.cpp
@@ -118,6 +118,51 @@ TEST_F(CLASSNAME(TestLexerLookaheadFixture, RMW_IMPLEMENTATION), test_peek2)
   EXPECT_EQ(RCL_LEXEME_FORWARD_SLASH, lexeme2);
 }
 
+TEST_F(CLASSNAME(TestLexerLookaheadFixture, RMW_IMPLEMENTATION), test_peek2_no_lexeme)
+{
+  rcl_ret_t ret;
+  rcl_lexer_lookahead2_t buffer;
+  SCOPE_LOOKAHEAD2(buffer, "~foo");
+
+  rcl_lexeme_t lexeme1 = RCL_LEXEME_NONE;
+  rcl_lexeme_t lexeme2 = RCL_LEXEME_NONE;
+
+  ret = rcl_lexer_lookahead2_peek2(&buffer, &lexeme1, &lexeme2);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_EQ(RCL_LEXEME_NONE, lexeme1);
+  EXPECT_EQ(RCL_LEXEME_NONE, lexeme2);
+}
+
+TEST_F(CLASSNAME(TestLexerLookaheadFixture, RMW_IMPLEMENTATION), test_peek2_no_lexeme_eof)
+{
+  rcl_ret_t ret;
+  rcl_lexer_lookahead2_t buffer;
+  SCOPE_LOOKAHEAD2(buffer, "~");
+
+  rcl_lexeme_t lexeme1 = RCL_LEXEME_NONE;
+  rcl_lexeme_t lexeme2 = RCL_LEXEME_NONE;
+
+  ret = rcl_lexer_lookahead2_peek2(&buffer, &lexeme1, &lexeme2);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_EQ(RCL_LEXEME_NONE, lexeme1);
+  EXPECT_EQ(RCL_LEXEME_NONE, lexeme2);
+}
+
+TEST_F(CLASSNAME(TestLexerLookaheadFixture, RMW_IMPLEMENTATION), test_peek2_eof)
+{
+  rcl_ret_t ret;
+  rcl_lexer_lookahead2_t buffer;
+  SCOPE_LOOKAHEAD2(buffer, "");
+
+  rcl_lexeme_t lexeme1 = RCL_LEXEME_NONE;
+  rcl_lexeme_t lexeme2 = RCL_LEXEME_NONE;
+
+  ret = rcl_lexer_lookahead2_peek2(&buffer, &lexeme1, &lexeme2);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_EQ(RCL_LEXEME_EOF, lexeme1);
+  EXPECT_EQ(RCL_LEXEME_EOF, lexeme2);
+}
+
 TEST_F(CLASSNAME(TestLexerLookaheadFixture, RMW_IMPLEMENTATION), test_eof)
 {
   rcl_ret_t ret;