Page MenuHomePhabricator

Memory error in SIMPLEXpress Unit Parser
Closed, CompletedPublic18 Energy Points (d+f*r)

Description

Jenkins is failing with undefined behavior, related to an error in some aspect of memory handling in SIMPLEXpress:

NOTE: Links are unrecoverable.

On the Release builds, GCC on Focal and Hirsute are displaying the following compiler warning:

In file included from /matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/include/simplexpress/char_sets.hpp:50,
                 from /matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/src/char_sets.cpp:1:
/matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/../../onestring/onestring/include/onestring/onechar.hpp: In static member function 'static bool char_sets::digit(onechar, int)':
/matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/../../onestring/onestring/include/onestring/onechar.hpp:326:31: error: array subscript [-25, -1] is outside array bounds of 'const char [2]' [-Werror=array-bounds]
  326 |                 switch (cstr[0] & 0xF0) {
      |                         ~~~~~~^
/matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/../../onestring/onestring/include/onestring/onechar.hpp:286:38: error: array subscript [-25, -1] is outside array bounds of 'const char [2]' [-Werror=array-bounds]
  286 |                         return memcmp(this->internal, cmp, this->size);
      |                                ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[4]: *** [CMakeFiles/simplexpress.dir/build.make:82: CMakeFiles/simplexpress.dir/src/char_sets.cpp.o] Error 1
make[4]: Leaving directory '/matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/build_temp/Release'
make[3]: *** [CMakeFiles/Makefile2:95: CMakeFiles/simplexpress.dir/all] Error 2
make[3]: Leaving directory '/matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/build_temp/Release'
make[2]: *** [Makefile:103: all] Error 2
make[2]: Leaving directory '/matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source/build_temp/Release'
make[1]: *** [../build_system/inner.mk:105: release] Error 2
make[1]: Leaving directory '/matrix/compiler/gcc/label/mpm-hirsute/simplexpress/simplexpress-source'
make: *** [build_system/outer.mk:125: simplexpress] Error 2

Although the error message appears to point to Onestring [Project], it's probable that something is wrong with the code in SIMPLEXpress [Project]. array subscript [-25, -1] is nonsense, and "if it's weird, it's memory." I suspect something wrong with a pointer, or otherwise uninitialized memory being accessed in some fashion, almost certainly in SIMPLEXpress.

The Debug builds don't have *any* compiler warnings, even on the same GCC versions on Focal and Hirsuite as produced the above. However, they all fail to pass Valgrind:

valgrind.out
===== [Specifier Parser] =====
Pass 1 of 1
==147097== Invalid read of size 8
==147097==    at 0x40DFC6: onechar::operator=(onechar const&) (../../onestring/onestring/include/onestring/onechar.hpp:372)
==147097==    by 0x42389A: onestring::assign(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:824)
==147097==    by 0x423713: onestring::operator=(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:1457)
==147097==    by 0x42282B: UnitParser::chop(onestring&) (src/unit_parser.cpp:466)
==147097==    by 0x421B99: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:309)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x404E92: main (main.cpp:74)
==147097==  Address 0x514b450 is 0 bytes inside a block of size 64 free'd
==147097==    at 0x483D74F: operator delete[](void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==147097==    by 0x4080D0: onestring::clear() (../../onestring/onestring/include/onestring/onestring.hpp:879)
==147097==    by 0x42385F: onestring::assign(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:822)
==147097==    by 0x423713: onestring::operator=(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:1457)
==147097==    by 0x42282B: UnitParser::chop(onestring&) (src/unit_parser.cpp:466)
==147097==    by 0x421B99: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:309)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==  Block was alloc'd at
==147097==    at 0x483C583: operator new[](unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==147097==    by 0x407DFA: onestring::allocate(unsigned long) (../../onestring/onestring/include/onestring/onestring.hpp:180)
==147097==    by 0x40D7B1: onestring::onestring(onestring const&) (../../onestring/onestring/include/onestring/onestring.hpp:126)
==147097==    by 0x421A5A: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:296)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x404E92: main (main.cpp:74)
==147097== 
==147097== Invalid read of size 8
==147097==    at 0x40DFE5: onechar::operator=(onechar const&) (../../onestring/onestring/include/onestring/onechar.hpp:373)
==147097==    by 0x42389A: onestring::assign(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:824)
==147097==    by 0x423713: onestring::operator=(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:1457)
==147097==    by 0x42282B: UnitParser::chop(onestring&) (src/unit_parser.cpp:466)
==147097==    by 0x421B99: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:309)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x404E92: main (main.cpp:74)
==147097==  Address 0x514b450 is 0 bytes inside a block of size 64 free'd
==147097==    at 0x483D74F: operator delete[](void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==147097==    by 0x4080D0: onestring::clear() (../../onestring/onestring/include/onestring/onestring.hpp:879)
==147097==    by 0x42385F: onestring::assign(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:822)
==147097==    by 0x423713: onestring::operator=(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:1457)
==147097==    by 0x42282B: UnitParser::chop(onestring&) (src/unit_parser.cpp:466)
==147097==    by 0x421B99: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:309)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==  Block was alloc'd at
==147097==    at 0x483C583: operator new[](unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==147097==    by 0x407DFA: onestring::allocate(unsigned long) (../../onestring/onestring/include/onestring/onestring.hpp:180)
==147097==    by 0x40D7B1: onestring::onestring(onestring const&) (../../onestring/onestring/include/onestring/onestring.hpp:126)
==147097==    by 0x421A5A: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:296)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x404E92: main (main.cpp:74)
==147097== 
==147097== Invalid read of size 1
==147097==    at 0x4842B60: memmove (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==147097==    by 0x40DFF2: onechar::operator=(onechar const&) (../../onestring/onestring/include/onestring/onechar.hpp:373)
==147097==    by 0x42389A: onestring::assign(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:824)
==147097==    by 0x423713: onestring::operator=(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:1457)
==147097==    by 0x42282B: UnitParser::chop(onestring&) (src/unit_parser.cpp:466)
==147097==    by 0x421B99: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:309)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==  Address 0x514b458 is 8 bytes inside a block of size 64 free'd
==147097==    at 0x483D74F: operator delete[](void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==147097==    by 0x4080D0: onestring::clear() (../../onestring/onestring/include/onestring/onestring.hpp:879)
==147097==    by 0x42385F: onestring::assign(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:822)
==147097==    by 0x423713: onestring::operator=(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:1457)
==147097==    by 0x42282B: UnitParser::chop(onestring&) (src/unit_parser.cpp:466)
==147097==    by 0x421B99: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:309)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==  Block was alloc'd at
==147097==    at 0x483C583: operator new[](unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==147097==    by 0x407DFA: onestring::allocate(unsigned long) (../../onestring/onestring/include/onestring/onestring.hpp:180)
==147097==    by 0x40D7B1: onestring::onestring(onestring const&) (../../onestring/onestring/include/onestring/onestring.hpp:126)
==147097==    by 0x421A5A: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:296)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)
==147097==    by 0x432134: TestManager::run_test(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433FB1: TestSuite::run_tests() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x4326F3: TestManager::run_suite(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x433E7C: TestManager::runall() (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x43F2D1: GoldilocksShell::command(int, char**, unsigned int) (in /home/jason/Code/Repositories/simplexpress/simplexpress-tester/bin/Debug/simplexpress-tester)
==147097==    by 0x404E92: main (main.cpp:74)
==147097== 
[!] Assert Ok(c, c) == Ok(c, )
TEST FAILED

The most important clue from this is the top of the stack trace on all three Valgrind errors:

==147097== Invalid read of size X
...
==147097==    by 0x40DFF2: onechar::operator=(onechar const&) (../../onestring/onestring/include/onestring/onechar.hpp:373)
==147097==    by 0x42389A: onestring::assign(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:824)
==147097==    by 0x423713: onestring::operator=(onechar const&) (../../onestring/onestring/include/onestring/onestring.hpp:1457)
==147097==    by 0x42282B: UnitParser::chop(onestring&) (src/unit_parser.cpp:466)
==147097==    by 0x421B99: UnitParser::specifier_parser(onestring const&) (src/unit_parser.cpp:309)
==147097==    by 0x4292DA: TestSpecifierParser::run() (include/simplexpress/unit_parser_test.hpp:154)

I'd focus primarily on src/unit_parser.cpp:466, although the Onestring part of the traceback may provide some clues as to where the usage in SIMPLEXpress is going wrong.

As an aside, it may be necessary to add additional safeguards to the Onestring API to prevent the erroneous usage, or to at least make it fail in a more deterministic way.

Details

Task Type
Bug
Proposed Urgency
5: Critical
Gravity
g5: Critical
Distance
d3: Within Half Sprint
Friction
f3: Off-Road
Relativity
r3: Moderate
Volatility (Caught At)
v2: SQA/Testing
Origin
o1: Coding

Revisions and Commits

rS SIMPLEXpress
Audit RequiredD440

Event Timeline

jcmcdonald changed the task status from Proposed to Open.Apr 23 2021, 1:32 PM
jcmcdonald assigned this task to wdede.
jcmcdonald triaged this task as p4: Now priority.
jcmcdonald changed Gravity from Triage Gravity to g5: Critical.
jcmcdonald set the point value for this task to 5.
jcmcdonald changed Relativity from Triage Relativity to r3: Moderate.
jcmcdonald changed Friction from Triage Friction to f3: Off-Road.
jcmcdonald added a subscriber: wdede.

@wdede I'm here if you need help with this; it's in the ballpark of the usual sort of bugs I solve. However, it would be good practice for you to try and solve it yourself. Have fun!

jcmcdonald set Distance to d3: Within Half Sprint.Jul 3 2021, 12:21 PM
jcmcdonald changed the point value for this task from 5 to 24.
rperez moved this task from Backlog to Next Sprint on the Onestring [Project] board.
rperez moved this task from Next Sprint to Current Sprint on the Onestring [Project] board.
jcmcdonald changed the point value for this task from 24 to 18.Jul 3 2021, 5:22 PM

@rperez Please create a subtask of this task about adding the additional error checking to Onestring.

This task really should only be closed once you have that implemented.

jcmcdonald changed the task status from Open to Pending Review.Oct 16 2021, 10:16 AM

Remember to actually create Differential Revisions for your work. I know you pushed to branches, but you actually need to run the following to create the Differentials that can be reviewed.

git checkout devel
git pull origin devel

git checkout the_branch_with_your_work
arc diff
galmonte changed the task status from Pending Review to Open.Mar 19 2022, 2:03 PM
galmonte claimed this task.
galmonte changed the status of subtask T1493: Fix related functions that calls substr() with wrong values. from Pending Review to Open.
galmonte added a subscriber: rperez.