Fix buffer overflow in argument parsing caused by lexer returning length beyond length of string (#979) (#981)

* Test that lexer never returns length longer than string

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* Fix bug where lexer returned length longer than string

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* Test that peeking 2 ahead never goes beyond NONE or EOF

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

* Stop peeking if the first lexeme is NONE or EOF

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
(cherry picked from commit 392f0d3ae6a29d4b88689b954b14f19716e44233)

Co-authored-by: Shane Loretz <sloretz@openrobotics.org>

rcl/include/rcl/lexer.h

@@ -86,6 +86,7 @@ typedef enum rcl_lexeme_t
  * 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>

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
+      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) {

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]) {

rcl/test/rcl/test_lexer.cpp

@@ -45,7 +45,8 @@ public:
     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)
 

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;