search

facebookexperimental / reverie

An ergonomic and safe syscall interception framework for Linux.
Other
563 stars 24 forks source link

Missing license attribution for `gdbstub` derived code #9

Closed daniel5151 closed 2 years ago

daniel5151 commented 2 years ago

Hey there,

It looks like reverie is using a fork of my gdbstub library without including the appropriate MIT license attribution (as per the license terms here). Based on a quick glance through the code, the forks to be pretty old, but there's nonetheless still some substantial chunks of my original implementation in there...

The relevant code in reverie lives under the following project subdirectory.

https://github.com/facebookexperimental/reverie/tree/15d2f6141137177e8fd292936dd01f63f3535a7f/reverie-ptrace/src/gdbstub

There are many examples of identical code, but one simple one in hex.rs:

https://github.com/facebookexperimental/reverie/blob/15d2f6141137177e8fd292936dd01f63f3535a7f/reverie-ptrace/src/gdbstub/hex.rs#L265-L288

vs. my implementation at

https://github.com/daniel5151/gdbstub/blob/dev/0.6/src/protocol/common/hex.rs#L11-L36

A more GDB RSP specific one is here:

https://github.com/facebookexperimental/reverie/blob/15d2f6141137177e8fd292936dd01f63f3535a7f/reverie-ptrace/src/gdbstub/packet.rs#L54-L72

vs. my implementation at

https://github.com/daniel5151/gdbstub/blob/0.4.5/src/protocol/packet.rs#L38-L56

jasonwhite commented 2 years ago

Hey Daniel, thanks for pointing out this mistake! We're definitely not trying to take credit for your excellent work. I'll fix that ASAP.

There should probably be two copyright headers for some files since we made quite a few significant changes. We initially tried to use your gdbstub crate as-is, but ran into problems integrating it with our async-ptrace implementation. We had to make a lot of changes to get around that limitation and so there shouldn't be too much in common. I'll go through everything and add the copyright attribution to the files where it makes sense.

I don't think we looked into using gdbstub again since https://github.com/daniel5151/gdbstub/issues/36 was solved. I'd love to circle back and get rid of our version completely if it's possible now.

daniel5151 commented 2 years ago

Hey Jason, all good!

I've got a habit of occasionally searching GitHub for gdbstub to see if any new projects are using it, and just happened to stumble across this one. Always happy to see more folks finding my niche little library useful :)

And yeah, if you do happen to have some spare cycles to get back in-sync with upstream gdbstub now that the async issue has been resolved, that'd be awesome! The library's gained a bunch of functionality over the past few months, and I'd be super keen to integrate any extensions y'all have developed as well.

Plus, I've really been dragging my feet on finally publishing gdbtsub 0.6, so maybe this'll be some good motivation to finally do that 😄

Thanks again!

jasonwhite commented 2 years ago

The library's gained a bunch of functionality over the past few months, and I'd be super keen to integrate any extensions y'all have developed as well.

That's great to hear! I don't think we have additional GDB extensions implemented above what is already implemented in (your) gdbstub.

Plus, I've really been dragging my feet on finally publishing gdbtsub 0.6, so maybe this'll be some good motivation to finally do that 😄

Alas, Reverie doesn't work on stable yet, so there's no requirement for gdbstub 0.6 to be published to crates.io yet. Time is the more limiting factor here. I'll try to contribute back fixes if/when we can get rid of our own version of gdbstub. :)

Please let me know if you believe I missed anything in https://github.com/facebookexperimental/reverie/pull/10. Otherwise, I plan on merging it as-is. Thanks!

daniel5151 commented 2 years ago

That's great to hear! I don't think we have additional GDB extensions implemented above what is already implemented in (your) gdbstub.

I don't think I've gotten around to implementing the various fork/vfork/clone stop reasons upstream, or the thread exit reason. Not that they'd be hard to implement, but no consumers of the library have needed them yet, so they haven't been implemented ¯\_(ツ)_/¯

Please let me know if you believe I missed anything in #10. Otherwise, I plan on merging it as-is. Thanks!

LGTM :+1:

Thanks again for the quick response and resolution!