sanitizeSlashes out-of-bounds string access

[richmattes]

Environment

• OS Version: Fedora 35 x86_64 (gcc-11.2.1)
• Source build: ign-common3 and ign-common4 branches affected

Description

The sanitizeSlashes lambda inside of ign::common::joinPaths (src/Filesystem.cc) attempts to remove duplicate occurrences of the path separator character at the beginning and end of a string. It does so by iterating over the string's characters forward from the beginning, and then backward from the end, until it reaches a character that is not a path separator variable, and then deletes characters leading up to the last path separator detected.

Under certain conditions, such as when the input string only contains one or more path separator variables, the loop iterating backwards from the end of the string will get to position 0, and then on the next iteration wrap the loop counter and attempt to access an element of the string way out of the string's bounds. I believe in the forward case, the loop iterating forward hits the null terminator at the end of the string, and will not continue to access any of the string out of bounds.

When compiled with -D_GLIBCXX_ASSERTIONS, an assertion is added to the C++ standard library for out-of-bounds accesses with methods like operator[](). The FilesystemTest.basename unit test triggers one of these assertions in the UNIT_Filesystem_TEST binary. When triggered, the binary aborts and the suite fails.

• Expected behavior: Tests build and run when -D_GLIBCXX_ASSERTIONS is set
• Actual behavior: Test aborts due to out-of-bounds access

Steps to reproduce

1. git checkout ign-common4
2. CXXFLAGS=-D_GLIBCXX_ASSERTIONS cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug
3. make -C build UNIT_Filesystem_TEST
4. ./build/bin/UNIT_Filesystem_TEST
5. gdb -ex run ./build/bin/UNIT_Filesystem_TEST

Output

Program output:

...
[ RUN      ] FilesystemTest.basename
/usr/include/c++/11/bits/basic_string.h:1058: std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator[](std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference = char&; std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::size_type = long unsigned int]: Assertion '__pos <= size()' failed.
Aborted (core dumped)

Backtrace (Error can be seen in frame 6, index value)

(gdb) bt full
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44 <snip>
#1  0x00007ffff79348b3 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
No locals.
#2  0x00007ffff78e76a6 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
<snip>
#3  0x00007ffff78d17d3 in __GI_abort () at abort.c:79
<snip>
#4  0x0000000000425f02 in std::__replacement_assert (__file=0x7ffff7f6c7b0 "/usr/include/c++/11/bits/basic_string.h", __line=1058, __function=0x7ffff7f6c5f8 "std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator[](std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::size_type) [with _Ch"..., __condition=0x7ffff7f6c5e1 "__pos <= size()") at /usr/include/c++/11/x86_64-redhat-linux/bits/c++config.h:2660
No locals.
#5  0x00007ffff7f0ce8f in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::operator[] (this=0x7fffffffc7e0, __pos=18446744073709551615) at /usr/include/c++/11/bits/basic_string.h:1058
        __PRETTY_FUNCTION__ = "std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::reference std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator[](std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::size_type) [with _Ch"...
#6  0x00007ffff7f0fbe6 in operator() (__closure=0x7fffffffc86f, _path="/", _stripLeading=false) at /home/rich/workspace/ignition/ign-common/src/Filesystem.cc:293
        result = "/"
        replacement = 47 '/'
        index = 18446744073709551615
        leadingIndex = 1
#7  0x00007ffff7f0fcf7 in ignition::common::joinPaths (_path1="", _path2="home") at /home/rich/workspace/ignition/ign-common/src/Filesystem.cc:304
        sanitizeSlashes = {<No data fields>}
        path = ""
#8  0x00000000004284b7 in ignition::common::joinPaths<char [4], char [4]> (_path1="", _path2="home") at /home/rich/workspace/ignition/ign-common/include/ignition/common/Filesystem.hh:153
No locals.
#9  0x000000000041ab5d in FilesystemTest_basename_Test::TestBody (this=0x4b15f0) at /home/rich/workspace/ignition/ign-common/src/Filesystem_TEST.cc:498
...

[richmattes]

Changing the for loop condition to index < result.length() && result[index] == replacement in the for loop would break of the loop when wrap-around occurs, before attempting to access the out-of-bounds index.

[mjcarroll]

Good catch, and thanks for the the insights, I'm opening a PR with your fix.

[mjcarroll]

Incorporated in ign-common3 via #303, and ported to ign-common4 via #305. Thanks!