JohnTitor / mach2

Apache License 2.0
32 stars 12 forks source link

Can this replace/deprecate `mach`? #13

Closed thomcc closed 8 months ago

thomcc commented 1 year ago

Oh! I had no idea this existed, that's great. It's probably worth asking @fitzgen if they'd be willing to deprecate mach existing one in favor of this (or transfer ownership).

Last week I noticed how bad mach was, and took a stab at cleaning it up and updating it (I figured I'd have to publish it as a new version, so I didn't keep changes minimal and used new core::ffi stuff, etc), but I never quite got around to finishing. Had I known about this crate, I wouldn't have bothered with any of that!

I think nobody should be using that one, a low level crate like that can't really go unmaintained, and I don't know that everything it has is fully right (several constants changed, and structures seemed wrong for aarch64 macos). So we should see if we can make this more widely known, at the very least.

JohnTitor commented 1 year ago

It's probably worth asking @fitzgen if they'd be willing to deprecate mach existing one in favor of this (or transfer ownership).

I asked @fitzgen on https://github.com/fitzgen/mach/issues/63#issuecomment-962011388 and Twitter but they just ignored me :( I guess they aren't interested in the mach crate and its maintenance at all...

If you have a good idea, let me know! I'm happy to help things as long as I can :)

thomcc commented 1 year ago

We could submit a rustsec unmaintained notice: https://github.com/rustsec/advisory-db/blob/main/HOWTO_UNMAINTAINED.md. If it has any badly incorrect definitions (like wrong function sigs or struct layouts), a stronger advisory would be warranted.

Concerning, a lot of code still uses mach (>5M downloads compared to mach2 which has under 50k... less than 1/100th the amount).


So, it's worth considering if any of the issues it has are worth an advisory. Looking at my diff (which is mostly irrelevant), the most changes I made that were most concerning are:

  1. The mach_vm_read_list function taking a Rust array (mach_vm_read_entry_t is a typedef defined as [mach_vm_read_entry; VM_MAP_ENTRY_MAX as usize]) by value. This is clearly broken, and may not work at all (I didn't think so, but maybe the ABI lines up).

  2. A few changed constants, only semi-concerning ones are:

    • MACH_RCV_NOTIFY was 0x200 but the header has it as 0, and 0x200 is unused.
    • MACH_RCV_OVERWRITE was 0x1000 but header has it as 0, and 0x1000 is used by MACH_RCV_GUARDED_DESC. (Unsure if this difference matters but it is a difference)
  3. Stuff like thread_state but it seems so obviously wrong (clearly x86) that it wouldn't be an issue. Actually, I take this back because thread_state gets put into other structures (like context types), and people may only check on x86, so this could be bad.

  4. Missing #[repr(C, packed(4))], but seemingly not in ways that impact layout (I guess these are already align==4, although IMO these should still have the annotation since they do in C).

Of these 1 and 3 are the only really bad ones (and maybe 2), and even 1 may be mostly ABI compatible (although I honestly dunno what happens if you call these with weird types since they aren't quite normal C functions). 3 is pretty bad though, if people don't notice.

Either way, I guess either 1 or 3 would be enough for most -sys crates to get a real advisory.

Let's wait to see if we hear from @fitzgen though, since they might prefer we not do that while they're the maintainer (since it tends to cause a flood of issues, at least if there's no fix available).

Did you see anything else that's wrong in the updates you made for mach2?

fitzgen commented 1 year ago

I haven't maintained mach for quite some number of years, @gnzlbg has been the maintainer.

JohnTitor commented 1 year ago

Did you see anything else that's wrong in the updates you made for mach2?

No, I've passively-maintained this crate for the libc crate and most contributions are made by other contributors.

I haven't maintained mach for quite some number of years, @gnzlbg has been the maintainer.

As I mentioned in https://github.com/fitzgen/mach/issues/63#issuecomment-962011388, @gnzlbg is now out of contact and they haven't maintained any crates, including libc, ctest, mach, etc. That's why I mentioned you initially. Even if you don't maintain it anymore, you still have access to the repo and crates.io, and you could deprecate the crate (open an issue on rustsec advisory) or transfer the ownership.

JohnTitor commented 8 months ago

Let's say this can replace/deprecate mach, actually the rustsec advisory shows this as an alternative.