trifectatechfoundation / sudo-rs

A memory safe implementation of sudo and su.
Other
2.9k stars 79 forks source link

Binary serialization over pipes #741

Closed xy2i closed 1 year ago

xy2i commented 1 year ago

We use UnixStream::pair() a few times and reimplement binary ser/de each time. Add an abstraction that does the serde (BinSerDe) and buffer ceremony (BinPipe) for us.

Fixes #471.

codecov[bot] commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@6d40763). Click here to learn what that means. Patch coverage: 51.63% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #741 +/- ## ======================================= Coverage ? 55.28% ======================================= Files ? 72 Lines ? 9835 Branches ? 0 ======================================= Hits ? 5437 Misses ? 4398 Partials ? 0 ``` | [Files Changed](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/741?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) | Coverage Δ | | |---|---|---| | [src/exec/no\_pty.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/741?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvbm9fcHR5LnJz) | `0.00% <0.00%> (ø)` | | | [src/exec/use\_pty/backchannel.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/741?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvdXNlX3B0eS9iYWNrY2hhbm5lbC5ycw==) | `0.00% <0.00%> (ø)` | | | [src/exec/use\_pty/monitor.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/741?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2V4ZWMvdXNlX3B0eS9tb25pdG9yLnJz) | `0.00% <0.00%> (ø)` | | | [src/common/bin\_serde.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/741?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL2NvbW1vbi9iaW5fc2VyZGUucnM=) | `91.30% <91.30%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

xy2i commented 1 year ago

Thank you for your review !

I ran into an issue I feel like is blocking, which I couldn't work around (see the review above). If there's no good work around I'm okay closing this PR, but let me know.

pvdrz commented 1 year ago

@xy2i oh silly me... Yeah I forgot that const generics is very minimal right now. But this is no reason to not merge your PR. There might be an alternative but I'm not sure how well it works:

mod sealed {
    pub trait DeSerializeBytes {}

    impl<const N: usize> DeSerializeBytes for [u8; N] {}
}

pub trait DeSerialize {
    type Bytes: sealed::DeSerializeBytes;
    fn serialize(&self) -> Self::Bytes;
    fn deserialize(bytes: Self::Bytes) -> Self;
}

impl DeSerialize for i32 {
    type Bytes = [u8; std::mem::size_of::<Self>()];

    fn serialize(&self) -> Self::Bytes {
        self.to_ne_bytes()    
    }

    fn deserialize(bytes: Self::Bytes) -> Self {
        Self::from_ne_bytes(bytes)
    }
}

Let me know if you think that's good enough. Otherwise, we can merge this as it is I think :)

Thanks!

xy2i commented 1 year ago

That works, thanks !

pvdrz commented 1 year ago

Thanks for this!