Skip to content

Decouple linux process specifics from elf dump file output #34

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.

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

Merged
merged 9 commits into from
Jun 6, 2025

Conversation

dblnz
Copy link
Contributor

@dblnz dblnz commented Apr 14, 2025

Objective

I would like to use the ELF core dump generation on a system that does not have linux kernel.
The ELF core dump generation should work also on windows.
I need to create a core dump from the information I have about the guest running in a hypervisor isolated Hyperlight Sandbox (registers and guest memory).

Design decisions

  • Maintain the existent public interface so that this does not introduce breaking changes
  • Add the possibility to replace the Linux process as input for the ELF core dump creation
  • Move the linux specific logic including the ProcessView type to a separate linux module that can be conditionally compiled when the platform is linux.
    This was not needed, but I thought it would be easier to follow.
  • Make some of the internal types public so that they can be used by the custom process info source (implies modifying visibility and add required documentation)

Description

I defined a trait called ProcessInfoSource that would provide the necessary information the elfcore core dump output logic needs (pid, threads, regions, mapped_files, aux_vector, page_size).
The already defined ProcessView now implements this trait. The elfcore core dump output logic now takes a ProcessInfoSource trait object that provides all the needed information. This decouples the core dump generation from the ProcessView type that assumes a linux kernel.
To provide the custom process information source, the from_source method was added to CoreDumpBuilder to create a custom CoreDumpBuilder that takes the custom info from a provided source and memory reader.

Copy link
Member

@chris-oo chris-oo left a comment

Choose a reason for hiding this comment

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

The overall shape of the PR seems reasonable, but some minor nitpicks and questions about dead code.

dblnz added 7 commits April 25, 2025 14:14
- This step is part of the move of linux specific functionality to
separate file in preparation for decoupling the linux Process specific
logic from the output of ELF dump file

Signed-off-by: Doru Blânzeanu <[email protected]>
Signed-off-by: Doru Blânzeanu <[email protected]>
- define ProcessInfoSource trait that describes a process information
data relevant to an ELF dump
- implement the trait for the existing Linux ProcessView to preserve the current
behavior
- create a `Box<dyn ReadProcessMemory>` by calling the `ProcessView`
  method `create_memory_reader` when the `ProcessInfoSource` is known to
  be a `ProcessView` or expect the reader as an argument when the
  `CoreDumpBuilder` is created `from_source`
- add documentation and adjust visibility to types that now need to be public
for the crate users to use when they define their own ProcessInfoSource
implementation

Signed-off-by: Doru Blânzeanu <[email protected]>
- the crate supports creating core dumps from a custom given source
not from a Pid

Signed-off-by: Doru Blânzeanu <[email protected]>
@dblnz dblnz force-pushed the split-linux-impl-from-elfcore branch from 37af989 to d3152cb Compare April 25, 2025 14:50
@dblnz
Copy link
Contributor Author

dblnz commented Apr 25, 2025

I took care of the comments above and modified the changes to address them and marked the conversation as resolved.
I modified the history of this branch to be easier to follow commit by commit.
I also added some tests and support for non linux platforms so we can generate ELF core dumps from ProcessInfoSource on any platforms.

@dblnz
Copy link
Contributor Author

dblnz commented May 7, 2025

I added a commit that changes the crate internally from using trait objects to generics.
The consequence is we have some breaking changes:

  • the CoreDumpBuilder struct expects two generic arguments
  • the LinuxProcessMemoryReader (internal type that handles linux process memory reading) needs to be public now
  • to create a core dump for a linux process one needs to use CoreDumpBuilder::<ProcessView, LinuxProcessMemoryReader>::new(pid) instead of CoreDumpBuilder::new(pid)

Copy link
Member

@chris-oo chris-oo left a comment

Choose a reason for hiding this comment

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

other than the nested imports, LGTM. Happy to merge once that's fixed.

I assume you'll need a new release on crates.io? I'll see about also getting some folks on my team to review.

@dblnz
Copy link
Contributor Author

dblnz commented May 16, 2025

Thanks for the review.
A new release on crates.io would be great.

Signed-off-by: Doru Blânzeanu <[email protected]>
@dblnz dblnz force-pushed the split-linux-impl-from-elfcore branch from c07c8b3 to cef4c80 Compare May 16, 2025 11:34
@chris-oo chris-oo enabled auto-merge (squash) June 5, 2025 21:25
@chris-oo chris-oo disabled auto-merge June 6, 2025 15:53
@chris-oo chris-oo merged commit cebd66b into microsoft:main Jun 6, 2025
5 of 9 checks passed
chris-oo added a commit that referenced this pull request Jun 6, 2025
Release version 2.0 that includes a breaking change to support a trait
based method of building coredumps from #34.
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.

3 participants