luser / rust-test-assembler

A Rust crate containing a set of types for building complex binary streams.
MIT License
10 stars 4 forks source link

Modernize crate using newer Rust features #12

Open luser opened 3 years ago

luser commented 3 years ago

I originally wrote this crate in 2015, when the latest version of Rust was 1.3.0. There have been many wonderful improvements to Rust and Cargo in the intervening years, but since this crate hasn't been under active development (it mostly just works!) it hasn't taken advantage of them.

I'd be open to someone doing the work to modernize this crate. Ideally if we were to tweak the API at all we could polish it up and produce a 1.0 release.

Jake-Shadle commented 3 years ago

I just started using this crate yesterday, and I think I want to take a stab at this, but there is one part of the API design that I kind of want to completely refactor, namely to change Section to use &mut self instead of self, as right now there scenarios that this API design choice makes really tedious as self methods are harder to use when mutating persistent sections over multiple calls. Would you be amenable to such a change? If not I think I would still want a &mut self API, but could be a different type or something so that the current API can stay backwards compatible with existing code.

Little bit of background, I'm rewriting breakpad's crash handling and minidump generation in Rust, which includes adding as many of the existing tests in breakpad to validate behavior, which led me to the synth_elf code that you originally wrote, since goblin doesn't have anything for elf/etc test generation https://github.com/m4b/goblin/issues/185, and luckily this crate already existed, so it's mainly just the elf specific parts that I need to rewrite.

luser commented 3 years ago

@Jake-Shadle hey! I will take a peek at your PR soon. I'm open to compelling arguments around API design, I was fairly new to Rust when I started this so I wouldn't argue that what's currently here is the best design, only that it works. :) I agree that I've also stumbled over places where the current self behavior made doing something more annoying.

The only two things I care deeply about preserving are:

luser commented 3 years ago

Little bit of background, I'm rewriting breakpad's crash handling and minidump generation in Rust

swoon

Are you familiar with the prior art here? AIUI this crate is actively being used in Firefox for Linux: https://github.com/msirringhaus/minidump_writer_linux . @gabrielesvelto could provide more details.

Jake-Shadle commented 3 years ago

Oh darn, wasn't aware of that project since it doesn't appear to be published to crates.io, and Linux was the first platform I was porting since getting crash dump reporting while targeting x86_64-unknown-linux-musl was the thing that motivated me to finally start porting it to pure Rust rather than just wrapping it. https://github.com/EmbarkStudios/sentry-contrib-rust/tree/main/breakpad-sys

gabrielesvelto commented 3 years ago

@Jake-Shadle I'd be interested in knowing your use-cases and if you'd like to work on this together. We have a fairly detailed plan you'll find described in the bugs linked to bug 1588530 on our tracker. In a nutshell we'd like the resulting crates to be as self-contained as possible so that they can be used by the wider ecosystem. Specifically we'd like to replace the client-side parts of breakpad with separate crates for:

  1. exception handling
  2. minidump generation
  3. crash report generation
  4. crash report submission

Breakpad includes functionality for the first three use-cases but all bundled up so the various pieces can't be used stand-alone. We have use cases for stand-alone exception handlers and so does Sentry so we'd like to make the rewrite modular. As you've seen for now we have only written the Linux minidump writer (with the help of a contributor) but I plan on working on the exception handler and crash report generator within the next 3 months.

Jake-Shadle commented 3 years ago

@gabrielesvelto I'd love to work together on this! I think the idea of a modular approach for the separate pieces totally makes sense, for example we wouldn't care so much about the crash report generation or submission since we already have those pieces for/with Sentry.

It does seem like there is 1 design decision that might be problematic though, which is that crashes are only handled out of process, so the current code in the minidump_linux_writer has heap allocations in it. My approach was rather going to be to keep the same page allocations/stack allocations strategy used in breakpad to avoid the global heap so that the handler/generation could still be done in process, but still make it easy to spawn a crash handler process on demand to use instead. This does of course make the exception handling/minidump creation more complicated, but I think it would be a shame to lose the in-process handling and minidump creation just to make the implementation simpler, though I totally understand if that's not going to change.

Oh, and maybe there is a better place for this discussion than in this issue? :eyes:

gabrielesvelto commented 3 years ago

Avoiding in-process generation was a deliberate choice. We've had far too many problems with that which lead to lost crashes (or worse, recursive ones). So we're moving to have a very slim monitor process which will take the minidumps from outside of whatever process crashed across all platforms. That'll be crate-ified too. For discussing this further we can catch up either on Sentry's Discord server (#native-crashes channel) or chat.mozilla.org under the #crashreporting channel where Sentry people also hang out.