Skip to content

IO: fix the uninitialised bytes memcheck reports in the unit tests - #1461

Merged
rjb32 merged 4 commits into
mainfrom
remyboutonnet/memcheck-fixes
Nov 11, 2026
Merged

rjb32 merged 4 commits into
mainfrom
remyboutonnet/memcheck-fixes

Conversation

@rjb32

@rjb32 rjb32 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Fix the uninitialised bytes valgrind memcheck reports in the unit tests.

@rjb32

rjb32 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Note

AI-generated summary.

Summary

Goal. Make the unit tests run clean under valgrind memcheck. One run of all 1252 tests reported uninitialised bytes in 3 tests. One of them is in the engine: a dump written through FilePageWriter::seek holds stale or uninitialised bytes after the end of its last page.

Approach. FilePageWriter::flush always writes a whole page, and now zeroes the bytes past size() first, for every caller. nextPage() and finish() did this themselves and seek() did not. AlignedBuffer's moves now keep _capacity, so a page size other than 1 MiB works. The other two reports were test bugs.

Where to look.

  • io/fs/FilePageWriter.cpp:111 flush — the one place every page is written
  • io/fs/AlignedBuffer.h:39 move constructor and :53 move assignment — _capacity moves, assignment frees the old buffer
  • test/storage/dump/PropertyDumpFormatTest.cpp:89 — the pinned hash that changes, and why it may

FilePageWriter's seek, nextPage and finish all write through flush, which now zeroes the end of the page; the six dumpers that seek(0) and the new tests around them

 seek(offset)                 nextPage()                   finish()
   flush()                      flush()                      flush()
+    zero bytes past size()   -  zero bytes past size()    -  zero bytes past size()
     write capacity() bytes       write capacity() bytes       write capacity() bytes

AlignedBuffer lost its capacity on a move: FilePageWriter::open(path, 16384) returned a writer whose 16 KiB buffer reported 1 MiB, and the first page overflowed it. Every caller in storage/dump passes DumpConfig::PAGE_SIZE, which is 1 MiB, so no dump was ever affected. The new zero-tail test uses 16 KiB pages and crashed on it.

The two test fixes: FileWriterTest wrote a 256-byte array with 3 bytes set, and WrittenEntityCase had no PrintTo, so gtest printed its padding bytes. It now prints GetParam() = Plain_Node_Match_Match_Read.

Evidence

  • Before: ctest -T memcheck -j8 on a RELDEBUG build, 1252 tests, 49 min 46 s:

    test_storage_dump_part_statistics             Syscall param write(buf) points to uninitialised byte(s)
                                                    FilePageWriter::flush <- seek <- EdgeIndexerDumper::dump
                                                    created by posix_memalign in AlignedBuffer
    test_io_filewriter                            Syscall param write(buf) points to uninitialised byte(s)
    test_query_ir_subquery_written_entity_matrix  8118 errors: Use of uninitialised value in PrintBytesInObjectTo
    

    test_io_filepagewriter_zero_tail failed SeekWritesZerosAfterTheBytesOfItsPage (tailIsZero false), and test_io_aligned_buffer_move failed both cases (capacity() 1048576, expected 16384).

    After: the same run on this branch, 1275 tests, 32 min 16 s, 0 memcheck errors. test_query_parser_mutant (1267 s under memcheck) and test_query_ir_fuzz_hang (its own 120 s timeout) were excluded, and --max-threads=2048 let jobs_test start its 1024 workers. Both new tests pass, natively and under valgrind.

  • The changed hash. PropertyDumpFormatTest.stringDumpBytes pins a hash of the string dump. I dumped it with the old and the new writer and compared the 6 MiB files:

    only page 5 differs, offsets 8064 to 1048572   (486010 bytes)
    new:  zeros from offset 8061 to the end of the page
    old:  those 1040512 bytes are a copy of page 4 at the same offsets
    

    The load round trip in the same test passes on both, so the format is unchanged and DumpConfig::VERSION stays.

Merge Danger

Door: two-way

Blast Radius: dumps

Dumps from the six dumpers that call seek(0) change on disk only in the bytes after the end of the page seek(0) flushes. The loaders never read those bytes, and dumps written before this change still load. Those bytes used to be a copy of an earlier page or uninitialised memory, so two dumps of one graph could differ.

@rjb32
rjb32 merged commit 66245d0 into main Nov 11, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant