Skip to content

Improve handling for invalid core files #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
godlygeek wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −16
Open

Improve handling for invalid core files #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/358.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Prevent reading past the end of a truncated or malformed core file's ``NT_FILE`` note.
43 changes: 27 additions & 16 deletions src/pystack/_pystack/corefile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,16 +278,18 @@ parseCoreSiginfo(const NoteData& note_data, CoreCrashInfo* result)
}

static StatusCode
parseCoreFileNote(Elf* core, const NoteData& note_data, std::vector<CoreVirtualMap>& result)
parseNtFileNote(const NoteData& note_data, std::vector<CoreVirtualMap>& result)
{
Elf_Data* data = note_data.data;
assert(data != NULL);

const size_t ulong_size = gelf_fsize(note_data.elf, ELF_T_ADDR, 1, EV_CURRENT);
if (ulong_size <= 0) {
LOG(ERROR) << "Cannot determine the size of 'long' for ELF file";

// The note holds a header of 2 longs, then N sets of 3 longs, then N null terminated names.

@blin blin Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!data->d_buf || data->d_size < 2 * ulong_size) {
LOG(ERROR) << "Failed to parse file note data: note is too small";
return StatusCode::ERROR;
}

const char* ptr = static_cast<const char*>(data->d_buf);
const char* end = static_cast<const char*>(data->d_buf) + data->d_size;

Expand All @@ -302,30 +304,39 @@ parseCoreFileNote(Elf* core, const NoteData& note_data, std::vector<CoreVirtualM
return StatusCode::ERROR;
}

// File names are stored at the end of the main table
const char* filename_table_start = ptr + count * entry_size;
const char* filename_table_ptr = filename_table_start;
const char* filename_table_ptr = ptr + count * entry_size;

@blin blin Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


std::vector<CoreVirtualMap> parsed;
parsed.reserve(count);
for (size_t i = 0; i < count; ++i) {
// Read the data for a single entry
uintptr_t mstart, mend, moffset;
read_obj(&ptr, &mstart, ulong_size);
read_obj(&ptr, &mend, ulong_size);
read_obj(&ptr, &moffset, ulong_size);

// Fetch the corresponding file name from the file name table
std::string filename(filename_table_ptr);
result.emplace_back(CoreVirtualMap{mstart, mend, 0, "", moffset * page_size, "", 0, filename});
uintptr_t offset;
if (__builtin_mul_overflow(moffset, page_size, &offset)) {

@blin blin Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, there is no clamping mul until C++26? That's a bit of a shame.

No action necessary, I was just a bit surprised by the use of a compiler intrinsic.

LOG(ERROR) << "Failed to parse file note data: file offset is out of range";
return StatusCode::ERROR;
}

// Advance the file name table pointer
const char* next_filename =
// Fetch the corresponding file name from the file name table
const char* filename_end =
static_cast<const char*>(memchr(filename_table_ptr, '\0', end - filename_table_ptr));
if (next_filename == nullptr) {
if (filename_end == nullptr) {
LOG(ERROR) << "Failed to parse file note data: file name table ended too soon";

@blin blin Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return StatusCode::ERROR;
}
filename_table_ptr = next_filename + 1;

std::string filename(filename_table_ptr, filename_end);
parsed.emplace_back(CoreVirtualMap{mstart, mend, 0, "", offset, "", 0, filename});

// Advance the file name table pointer
filename_table_ptr = filename_end + 1;
}

result = std::move(parsed); // Only modify the caller's vector once we've succeeded.
return StatusCode::SUCCESS;
}

Expand All @@ -337,12 +348,12 @@ CoreFileExtractor::extractMappedFiles() const
LOG(DEBUG) << "Extracting mapped files from core file note";

for (const auto& note_data : getNoteData(elf, NT_FILE, ELF_T_XWORD)) {
if (parseCoreFileNote(elf, note_data, result) != StatusCode::ERROR) {
if (parseNtFileNote(note_data, result) != StatusCode::ERROR) {
LOG(DEBUG) << "Mapped files found in core file note";
return result;
}
}
LOG(DEBUG) << "Mapped files could not be found in core file note";
LOG(DEBUG) << "Mapped files could not be retrieved from core file note";
return result;
}

Expand Down
Loading