Improve handling for invalid core files

[godlygeek]

Prevent reading past the end of a truncated or malformed core file's NT_FILE note.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.75%. Comparing base (a0ec380) to head (5cec815).

Files with missing lines	Patch %	Lines
src/pystack/_pystack/corefile.cpp	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #358      +/-   ##
==========================================
+ Coverage   78.19%   78.75%   +0.56%     
==========================================
  Files          58       58              
  Lines        6690     6709      +19     
  Branches      630      632       +2     
==========================================
+ Hits         5231     5284      +53     
+ Misses       1459     1425      -34     

Flag	Coverage Δ
cpp	78.75% <93.54%> (+0.56%)	⬆️
python	78.75% <93.54%> (+0.56%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[blin]

Consider linking to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/binfmt_elf.c?h=v4.18#n1594 or https://github.com/torvalds/linux/blob/v4.18/fs/binfmt_elf.c#L1594 either next to the comment or in the commit message, it would make it easier to understand where the format comes from, and also to compare the comment/implementation to the source of the format.

[blin]

Please add a test. One in a style similar to test_core_analyzer_rejects_incompatible_core_format would be okay:

def test_core_analyzer_rejects_malformed_nt_file(
    tmpdir: Path, caplog: LogCaptureFixture
) -> None:
    core = bytearray((CORE_FILE_PATHS / "segfault.core").read_bytes())
    ulong_size = 8
    desc, descsz = (2148, 451)
    struct.pack_into("<Q", core, desc, int((descsz-(2*ulong_size))/(3*ulong_size)))
    malformed_core = Path(tmpdir) / "malformed_nt_file.core"
    malformed_core.write_bytes(bytes(core))

    caplog.set_level(logging.ERROR)
    CoreFileAnalyzer(str(malformed_core)).extract_maps()
    assert "Failed to parse file note data: file name table ended too soon" in caplog.text

(2148, 451) are a one-off computation of the real offset and size, I don't expect segfault.core to change so wouldn't mind keeping it in as is. Doing a bunch of offset arithmetic in a test helper or using pyelftools would also be fine with me (and would be a lot more readable and robust).

Re-writing NULL terminator into some character would also be fine, and might be a bit cleaner, but would still require finding desc and descsz.

[godlygeek]

using pyelftools would also be fine with me (and would be a lot more readable and robust).

I do think that'd be better, but I don't really expect this to need to change much, so I'm just going to go with your test verbatim and not worry about doing something more robust unless we ever need to touch it again. I expect the only thing we'll ever need to change in this test is possibly changing the string we're asserting on.

[godlygeek]

FWIW, the reason I didn't include a test myself is that I was having trouble getting one small enough that I was willing to include it. I was trying to do an end-to-end test where pystack couldn't find the info it needed without looking at the notes, so it would try to look at the notes and then fail, and I wound up with about 50 lines of too-complex test code for it. Calling CoreFileAnalyzer.extract_maps() directly is so obviously better that I don't know why I didn't think of it myself 😅

So, very helpful review comment, haha!

[blin]

nit: Consider renaming to filename_start, it will make later uses with filename_end cleaner.

[godlygeek]

With one name, we have to choose which of two things to make clearer:

• That there is a table of file names which we are iterating through
• That our pointer points to the start of the current name cstring in that table

I think the rename you're suggesting makes the more obvious thing clearer, at the cost of obscuring the much less obvious and weirder thing - that we're iterating through N null-terminated strings all concatenated to each other.

How about calling our main iterator into the table next_file, and our pointer to the end of the first C string it points to next_file_end?

[blin]

Fair point.

I like "next" for kind of linked-list semantics. Would be nice to have "filename" instead of "file", and would be nice to have _start suffix on the first variable for symmetry with _end suffix, which I think does not affect the linked-list semantics.

My preference order would be:

1. next_filename_start/next_filename_end
2. next_filename/next_filename_end
3. next_file_start/next_file_end
4. next_file/next_file_end

[godlygeek]

OK, I've pushed a fixup commit that I believe addresses all of your concerns. If you're happy with the results, lmk and I'll squash and merge.