Fix buffer overflow in argument parsing caused by lexer returning length beyond length of string

[sloretz]

While running rcl's tests under address sanitizer I noticed a buffer overflow in test_arguments here:

rcl/rcl/test/rcl/test_arguments.cpp

Line 239 in 71baed4

EXPECT_FALSE(are_valid_ros_args({"--ros-args", "-r", "~"}));

Address sanitizer output in test

    Start 49: test_arguments__rmw_fastrtps_cpp

49: Test command: /usr/bin/python3.10 "-u" "/home/sloretz/ws/ros2/install/ament_cmake_test/share/ament_cmake_test/cmake/run_test.py" "/home/sloretz/ws/ros2/build/rcl/test_results/rcl/test_arguments__rmw_fastrtps_cpp.gtest.xml" "--package-name" "rcl" "--output-file" "/home/sloretz/ws/ros2/build/rcl/ament_cmake_gtest/test_arguments__rmw_fastrtps_cpp.txt" "--env" "RMW_IMPLEMENTATION=rmw_fastrtps_cpp" "--append-env" "LD_LIBRARY_PATH=/home/sloretz/ws/ros2/build/rcl" "--command" "/home/sloretz/ws/ros2/build/rcl/test/test_arguments__rmw_fastrtps_cpp" "--gtest_output=xml:/home/sloretz/ws/ros2/build/rcl/test_results/rcl/test_arguments__rmw_fastrtps_cpp.gtest.xml"
49: Test timeout computed to be: 60
49: -- run_test.py: extra environment variables:
49:  - RMW_IMPLEMENTATION=rmw_fastrtps_cpp
49: -- run_test.py: extra environment variables to append:
49:  - LD_LIBRARY_PATH+=/home/sloretz/ws/ros2/build/rcl
49: -- run_test.py: invoking following command in '/home/sloretz/ws/ros2/build/rcl/test':
49:  - /home/sloretz/ws/ros2/build/rcl/test/test_arguments__rmw_fastrtps_cpp --gtest_output=xml:/home/sloretz/ws/ros2/build/rcl/test_results/rcl/test_arguments__rmw_fastrtps_cpp.gtest.xml
49: Running main() from /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/src/gtest_main.cc
49: [==========] Running 43 tests from 1 test suite.
49: [----------] Global test environment set-up.
49: [----------] 43 tests from TestArgumentsFixture__rmw_fastrtps_cpp
49: [ RUN      ] TestArgumentsFixture__rmw_fastrtps_cpp.check_known_vs_unknown_args
49: [       OK ] TestArgumentsFixture__rmw_fastrtps_cpp.check_known_vs_unknown_args (3 ms)
49: [ RUN      ] TestArgumentsFixture__rmw_fastrtps_cpp.check_valid_vs_invalid_args
49: =================================================================
49: ==24050==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55e7ceb21242 at pc 0x7f3d5470a77f bp 0x7ffd4a35dd30 sp 0x7ffd4a35dd20
49: READ of size 1 at 0x55e7ceb21242 thread T0
49:     #0 0x7f3d5470a77e in rcl_lexer_analyze /home/sloretz/ws/ros2/src/ros2/rcl/rcl/src/rcl/lexer.c:612
49:     #1 0x7f3d5470bb83 in rcl_lexer_lookahead2_peek2 /home/sloretz/ws/ros2/src/ros2/rcl/rcl/src/rcl/lexer_lookahead.c:148
49:     #2 0x7f3d546f4625 in _rcl_parse_remap_begin_remap_rule /home/sloretz/ws/ros2/src/ros2/rcl/rcl/src/rcl/arguments.c:1631
49:     #3 0x7f3d546f6387 in _rcl_parse_remap_rule /home/sloretz/ws/ros2/src/ros2/rcl/rcl/src/rcl/arguments.c:1850
49:     #4 0x7f3d546e6382 in rcl_parse_arguments /home/sloretz/ws/ros2/src/ros2/rcl/rcl/src/rcl/arguments.c:363
49:     #5 0x55e7ce9f6d22 in are_valid_ros_args(std::vector<char const*, std::allocator<char const*> >) /home/sloretz/ws/ros2/src/ros2/rcl/rcl/test/rcl/test_arguments.cpp:211
49:     #6 0x55e7ce9fa67d in TestArgumentsFixture__rmw_fastrtps_cpp_check_valid_vs_invalid_args_Test::TestBody() /home/sloretz/ws/ros2/src/ros2/rcl/rcl/test/rcl/test_arguments.cpp:239
49:     #7 0x55e7ceaee8ed in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2433
49:     #8 0x55e7ceade469 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2469
49:     #9 0x55e7cea84d7f in testing::Test::Run() /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2508
49:     #10 0x55e7cea86254 in testing::TestInfo::Run() /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2684
49:     #11 0x55e7cea86f8c in testing::TestSuite::Run() /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2816
49:     #12 0x55e7ceaa3ef3 in testing::internal::UnitTestImpl::RunAllTests() /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:5338
49:     #13 0x55e7ceaf1d13 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2433
49:     #14 0x55e7ceae0faa in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2469
49:     #15 0x55e7ceaa0987 in testing::UnitTest::Run() /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/./src/gtest.cc:4925
49:     #16 0x55e7cea70b13 in RUN_ALL_TESTS() /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:2473
49:     #17 0x55e7cea709dc in main /home/sloretz/ws/ros2/install/gtest_vendor/src/gtest_vendor/src/gtest_main.cc:45
49:     #18 0x7f3d52e3bd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
49:     #19 0x7f3d52e3be3f in __libc_start_main_impl ../csu/libc-start.c:392
49:     #20 0x55e7ce9e11f4 in _start (/home/sloretz/ws/ros2/build/rcl/test/test_arguments__rmw_fastrtps_cpp+0x2d1f4)
49: 
49: 0x55e7ceb21242 is located 62 bytes to the left of global variable '*.LC128' defined in '/home/sloretz/ws/ros2/src/ros2/rcl/rcl/test/rcl/test_arguments.cpp' (0x55e7ceb21280) of size 46
49:   '*.LC128' is ascii string 'are_valid_ros_args({"--ros-args", "-r", "~"})'
49: 0x55e7ceb21242 is located 0 bytes to the right of global variable '*.LC127' defined in '/home/sloretz/ws/ros2/src/ros2/rcl/rcl/test/rcl/test_arguments.cpp' (0x55e7ceb21240) of size 2
49:   '*.LC127' is ascii string '~'
49: SUMMARY: AddressSanitizer: global-buffer-overflow /home/sloretz/ws/ros2/src/ros2/rcl/rcl/src/rcl/lexer.c:612 in rcl_lexer_analyze
49: Shadow bytes around the buggy address:
49:   0x0abd79d5c1f0: f9 f9 f9 f9 00 00 00 00 00 00 07 f9 f9 f9 f9 f9
49:   0x0abd79d5c200: 00 00 00 00 00 01 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
49:   0x0abd79d5c210: f9 f9 f9 f9 00 00 00 00 00 06 f9 f9 f9 f9 f9 f9
49:   0x0abd79d5c220: 02 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 06 f9 f9
49:   0x0abd79d5c230: f9 f9 f9 f9 02 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
49: =>0x0abd79d5c240: 00 06 f9 f9 f9 f9 f9 f9[02]f9 f9 f9 f9 f9 f9 f9
49:   0x0abd79d5c250: 00 00 00 00 00 06 f9 f9 f9 f9 f9 f9 03 f9 f9 f9
49:   0x0abd79d5c260: f9 f9 f9 f9 00 00 00 00 00 07 f9 f9 f9 f9 f9 f9
49:   0x0abd79d5c270: 06 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 02 f9
49:   0x0abd79d5c280: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
49:   0x0abd79d5c290: 00 00 02 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
49: Shadow byte legend (one shadow byte represents 8 application bytes):
49:   Addressable:           00
49:   Partially addressable: 01 02 03 04 05 06 07 
49:   Heap left redzone:       fa
49:   Freed heap region:       fd
49:   Stack left redzone:      f1
49:   Stack mid redzone:       f2
49:   Stack right redzone:     f3
49:   Stack after return:      f5
49:   Stack use after scope:   f8
49:   Global redzone:          f9
49:   Global init order:       f6
49:   Poisoned by user:        f7
49:   Container overflow:      fc
49:   Array cookie:            ac
49:   Intra object redzone:    bb
49:   ASan internal:           fe
49:   Left alloca redzone:     ca
49:   Right alloca redzone:    cb
49:   Shadow gap:              cc
49: ==24050==ABORTING
49: -- run_test.py: return code 1
49: -- run_test.py: generate result file '/home/sloretz/ws/ros2/build/rcl/test_results/rcl/test_arguments__rmw_fastrtps_cpp.gtest.xml' with failed test
49: -- run_test.py: verify result file '/home/sloretz/ws/ros2/build/rcl/test_results/rcl/test_arguments__rmw_fastrtps_cpp.gtest.xml'
2/3 Test #49: test_arguments__rmw_fastrtps_cpp ...........***Failed    0.36 sec

The lexer tries to match the lexeme TILDE_SLASH, but fails because the end of the string is reached. When matching a lexeme fails, the lexer uses a default transition to T_NONE. This transition increases the length of the lexeme by 1, with the idea being to make the length include the character which caused no lexeme to be found so it can be included in error messages. This behavior is a problem when the end of the string is reached, because any code trying to access the problematic string now reads 1 past the end of the string.

In this particular case, rcl_lexer_lookahead2_peek2() caused the overflow by trying to read a second lexeme starting at one past the end of the string.

This PR fixes the buffer overflow in two ways. First it makes sure the lexer never returns a length beyond the length of the string. Second, it makes rcl_lexer_lookahead2_peek2() stop peeking at the string if the first peek is NONE or EOF.

[sloretz]

CI (build: --packages-up-to rcl test: --packages-select rcl)

• Linux
• Linux-aarch64
• Windows

[ivanpauno]

I'm not super familiar with this code, but this seems reasonable to me

[sloretz]

@Mergifyio backport galactic foxy

[mergify]

backport galactic foxy

✅ Backports have been created

• #980 Fix buffer overflow in argument parsing caused by lexer returning length beyond length of string (backport #979) has been created for branch galactic
• #981 Fix buffer overflow in argument parsing caused by lexer returning length beyond length of string (backport #979) has been created for branch foxy