Run C++/Perlang-based tests with Valgrind #471

Closed perlun closed 2 weeks ago

Moved to GitLab

Please continue to the new version of this issue here: https://gitlab.perlang.org/perlang/perlang/-/issues/471. The GitHub page you are currently reading will not contain the latest information on this issue.

Now that we are adding more and more C++-based code in our standard library, we are highly susceptible to introduce memory leaks and/or double-free, buffer overruns and other "fun" errors in our code (because C++ is not a memory safe language, particularly when you use pointers and other C-language constructs).

Case study

Here's a recent example of this, where either I or GitHub Copilot (don't fully remember :slightly_smilingface:) incorrectly used the `lengthfield instead of the newly calculatedlengthvariable when allocating achar *` buffer for the newly allocated string:

std::shared_ptr<const String> UTF8String::operator+(const String& rhs) const
{
    size_t length = this->length_ + rhs.length();
    char *bytes = new char[length_ + 1];

    memcpy((void*)bytes, this->bytes_, this->length_);
    memcpy((void*)(bytes + this->length_), rhs.bytes(), rhs.length());
    bytes[length] = '\0';

    return from_owned_string(bytes, length);
}

Luckily for me, this gave me a SIGSEGV which is always better than the program "working" (despite it being broken).

Running it via Valgrind gave quite good details on exactly where the overrun happened:

~/git/perlang on  feature/support-ASCIIString-concatenation-in-compiled-mode [!] 
❯ valgrind --leak-check=yes /home/per/git/perlang/src/stdlib/cmake-build-debug/cctest print
==3143348== Memcheck, a memory error detector
==3143348== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==3143348== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==3143348== Command: /home/per/git/perlang/src/stdlib/cmake-build-debug/cctest print
==3143348== 
==3143348== Invalid write of size 1
==3143348==    at 0x484A433: memmove (vg_replace_strmem.c:1382)
==3143348==    by 0x157A62: perlang::UTF8String::operator+(perlang::String const&) const (utf8_string.cc:72)
==3143348==    by 0x132B63: TestPrintDouble_9223372036854775807() (print.cc:98)
==3143348==    by 0x1339C0: CcTest::Run() (cctest.h:142)
==3143348==    by 0x133944: main (cctest.cc:132)
==3143348==  Address 0x4d77198 is 0 bytes after a block of size 8 alloc'd
==3143348==    at 0x484220F: operator new[](unsigned long) (vg_replace_malloc.c:640)
==3143348==    by 0x1579F2: perlang::UTF8String::operator+(perlang::String const&) const (utf8_string.cc:69)
==3143348==    by 0x132B63: TestPrintDouble_9223372036854775807() (print.cc:98)
==3143348==    by 0x1339C0: CcTest::Run() (cctest.h:142)
==3143348==    by 0x133944: main (cctest.cc:132)
==3143348== 
==3143348== Invalid write of size 1
==3143348==    at 0x157A6E: perlang::UTF8String::operator+(perlang::String const&) const (utf8_string.cc:73)
==3143348==    by 0x132B63: TestPrintDouble_9223372036854775807() (print.cc:98)
==3143348==    by 0x1339C0: CcTest::Run() (cctest.h:142)
==3143348==    by 0x133944: main (cctest.cc:132)
==3143348==  Address 0x4d7719e is 6 bytes after a block of size 8 alloc'd
==3143348==    at 0x484220F: operator new[](unsigned long) (vg_replace_malloc.c:640)
==3143348==    by 0x1579F2: perlang::UTF8String::operator+(perlang::String const&) const (utf8_string.cc:69)
==3143348==    by 0x132B63: TestPrintDouble_9223372036854775807() (print.cc:98)
==3143348==    by 0x1339C0: CcTest::Run() (cctest.h:142)
==3143348==    by 0x133944: main (cctest.cc:132)

Bottom line

Once we have some kind of C++-based testing infrastructure in place, we should run all our tests through Valgrind (perhaps as a separate CI job or something). For now, we could actually add it to the Perlang compiler, perhaps by adding a new --valgrind parameter or something, which would make the compiler run our binaries via Valgrind. This would be useful when debugging issues like this. Note: Valgrind does cause the programs to run more slowly, so a separate CI job (perhaps even something that's only executed nightly or so) would make a certain bit of sense to avoid slowing down everyday work.

perlang-org / perlang