Construct a SandboxBuilder from its guest source - #1760
Conversation
`GuestBinary::Buffer` holds a `Vec<u8>`, so `GuestBinary` and `GuestEnvironment` carry no lifetime for the guest binary. A caller can hand its bytes over rather than keep them alive for as long as the sandbox. `ElfInfo` takes the payload by value, saving a copy of the guest binary when the bytes come from memory. Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
`SandboxBuilder::from_guest_file`, `from_guest_bytes` and `from_snapshot` name the source up front, and `build()` creates the sandbox. `guest_file`, `guest_bytes` and `guest_snapshot` set the source on an existing builder, so `new()` still serves callers that gather settings before the guest is known. The `build_from_*` methods become shorthand for naming a source and building in one call. Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Name the guest binary or snapshot when creating the builder, then call `build()`. Helpers that hand out a preconfigured builder carry their guest with them, and the test helpers take a closure that configures the builder rather than a builder value. Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
`SandboxBuilder::build` is the only way to build a sandbox, and the `from_*` constructors and `guest_*` setters are the only ways to name its source. So the `build_from_*` methods go, along with `new` and its `Default` impl, which could not name one, and `MultiUseSandbox::builder`, which handed out such a builder. Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
|
@ludfjig, @yoshuawuyts, PTAL :-) |
There was a problem hiding this comment.
Pull request overview
Refactors SandboxBuilder to require a guest source at construction, use build() as the terminator, and own guest byte buffers.
Changes:
- Adds source-aware constructors and setters.
- Updates ELF and executable ownership handling.
- Migrates tests, examples, fuzz targets, documentation, and changelog.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Summary |
|---|---|
src/hyperlight_host/tests/snapshot_goldens/fixtures.rs |
Updates golden fixture construction. |
src/hyperlight_host/tests/snapshot_goldens/checks.rs |
Updates golden snapshot loading. |
src/hyperlight_host/tests/sandbox_host_tests.rs |
Migrates host tests. |
src/hyperlight_host/tests/integration_test.rs |
Adapts integration test configuration. |
src/hyperlight_host/tests/common/mod.rs |
Updates shared sandbox helpers. |
src/hyperlight_host/src/sandbox/uninitialized.rs |
Owns guest buffers; GuestEnvironment still retains a borrowed init-data lifetime, contrary to the documented ownership goal. Moderate finding, 2 votes. |
src/hyperlight_host/src/sandbox/snapshot/mod.rs |
Accepts owned guest binaries. |
src/hyperlight_host/src/sandbox/snapshot/file/mod.rs |
Updates snapshot documentation. |
src/hyperlight_host/src/sandbox/snapshot/file_tests.rs |
Migrates snapshot tests. |
src/hyperlight_host/src/sandbox/initialized_multi_use.rs |
Removes the old builder factory. |
src/hyperlight_host/src/sandbox/host_funcs.rs |
Updates builder documentation. |
src/hyperlight_host/src/sandbox/builder.rs |
Adds source-aware builder construction. |
src/hyperlight_host/src/metrics/mod.rs |
Migrates metrics tests. |
src/hyperlight_host/src/mem/exe.rs |
Transfers executable input ownership. |
src/hyperlight_host/src/mem/elf.rs |
Retains owned ELF payloads. |
src/hyperlight_host/examples/tracing/main.rs |
Migrates tracing example. |
src/hyperlight_host/examples/tracing-otlp/main.rs |
Migrates OTLP tracing example. |
src/hyperlight_host/examples/tracing-chrome/main.rs |
Migrates Chrome tracing example. |
src/hyperlight_host/examples/metrics/main.rs |
Migrates metrics example. |
src/hyperlight_host/examples/map-file-cow-test/main.rs |
Migrates file-mapping example. |
src/hyperlight_host/examples/logging/main.rs |
Migrates logging example. |
src/hyperlight_host/examples/hello-world/main.rs |
Migrates hello-world example. |
src/hyperlight_host/examples/guest-debugging/main.rs |
Migrates debugging example. |
src/hyperlight_host/examples/func_ctx/main.rs |
Migrates function-context example. |
src/hyperlight_host/examples/crashdump/main.rs |
Migrates crashdump example. |
src/hyperlight_host/benches/benchmarks.rs |
Migrates benchmarks. |
README.md |
Updates the primary usage example. |
fuzz/fuzz_targets/host_print.rs |
Migrates fuzz initialization. |
fuzz/fuzz_targets/host_call.rs |
Migrates fuzz initialization. |
fuzz/fuzz_targets/guest_trace.rs |
Migrates fuzz initialization. |
fuzz/fuzz_targets/guest_call.rs |
Migrates fuzz initialization. |
docs/how-to-debug-a-hyperlight-guest.md |
Updates debugging documentation. |
CHANGELOG.md |
Documents owned guest buffers. |
Suppressed comments (2)
src/hyperlight_host/src/sandbox/builder.rs:228
- These setters can replace the source, so this behavior depends on the source at
buildtime rather than the constructor used. A builder created from a guest file and then changed withguest_snapshotstill errors here, while a snapshot builder changed withguest_filedoes not. Document the condition in terms of the current source.
/// Note: [`Self::build`] errors if this setting is set on a builder created
/// with [`Self::from_snapshot`], as the snapshot already contains the init data.
src/hyperlight_host/src/sandbox/builder.rs:267
- These setters can replace the source, so this behavior depends on the source at
buildtime rather than the constructor used. A builder created from a guest file and then changed withguest_snapshotstill errors here, while a snapshot builder changed withguest_filedoes not. Document the condition in terms of the current source.
/// Note: [`Self::build`] errors if this setting is set on a builder created
/// with [`Self::from_snapshot`], as the log level is already captured in the snapshot.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
yoshuawuyts
left a comment
There was a problem hiding this comment.
I'm surprised by the names chosen here? Are we expecting e.g. from_host_bytes/from_host_file or anything similar in the future? If not, the additional *_guest* addition to the paths feel superfluous.
If users are unsure what the encoding of the arguments ought to be, I feel like comments would be the right place to clarify that?
No, the intention was to make it clearer what "file" means. But I see your point.
Will do that |
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
yoshuawuyts
left a comment
There was a problem hiding this comment.
Some API changes stood out to me that I have questions about
| impl ElfInfo { | ||
| pub(crate) fn new(bytes: &[u8]) -> Result<Self> { | ||
| let elf = Elf::parse(bytes)?; | ||
| pub(crate) fn new(bytes: impl Into<Vec<u8>>) -> Result<Self> { |
There was a problem hiding this comment.
did you mean to change this? I don't believe Self::read_version_note changed it signature?
| Self::from_buf(contents) | ||
| } | ||
| pub fn from_buf(buf: &[u8]) -> Result<Self> { | ||
| pub fn from_buf(buf: impl Into<Vec<u8>>) -> Result<Self> { |
There was a problem hiding this comment.
Same here - is this change needed? I'm asking because I don't see any other uses for it that would suggest we need to make this change?
Naming the guest source
The builder used to start sourceless, with the terminator naming the guest:
It now takes the source up front, and
buildis the only terminator:from_bytesandfrom_snapshotare the other two constructors. A builder namesits source once, at construction, and nothing changes it afterwards.
from_filetakes the path of a guest ELF file,
from_bytesthe contents of one.Owning the guest bytes
GuestBinaryused to borrow:It now owns, and carries no lifetime:
A caller hands its bytes over rather than keeping them alive for as long as the
sandbox, and the builder stores them without a lifetime of its own.
GuestEnvironmentkeeps its'b, the lifetime of the borrowed init data.ElfInfotakes the payload by value, which drops a copy of the guest binarythat the borrowed form forced.
This is the only breaking change, and it carries a changelog entry.
Removed
SandboxBuilder::new().build_from_file(p)SandboxBuilder::from_file(p).build()SandboxBuilder::new().build_from_bytes(b)SandboxBuilder::from_bytes(b).build()SandboxBuilder::new().build_from_snapshot(s)SandboxBuilder::from_snapshot(s).build()MultiUseSandbox::builder()SandboxBuilder::from_file(p)and friendsSandboxBuilder::new()andSandboxBuilder::default()SandboxBuilder::newand itsDefaultimpl go too, as neither could name asource. All of these arrived unreleased in #1725 and #1746, so none needs a
changelog entry.
Commits
The four base commits build and test on their own. The fixups on top carry
review feedback and fold into them.