search

seL4 / microkit

Microkit - A simple operating system framework for the seL4 microkernel
Other
68 stars 37 forks source link

chore: apply clippy lints on Rust code #137

Closed wucke13 closed 1 week ago

wucke13 commented 2 weeks ago

There is a bunch of code flagged by clippy.

wucke13 commented 2 weeks ago

Worth pointing out: https://github.com/wucke13/microkit/blob/dev/improve-rust/tool/microkit/src/util.rs#L129-L143 is still unsafe (and likely has actual soundness holes for little vs big endian machines, as irrelevant as these computers may seem). There is multiple ways to address this; one would be https://lib.rs/crates/deku . I would be happy to contribute the code to get rid of the remaining two unsafe functions, but I don't want to make the effort if upstream is not interested in this change. Please let me know what you think, would you be OK to get deku (or any other thing you suggest) in to get rid of the unsafe bits?

Ivan-Velickovic commented 2 weeks ago

(and likely has actual soundness holes for little vs big endian machines, as irrelevant as these computers may seem)

Yes, that's why there is this check https://github.com/seL4/microkit/blob/eef6faff0783ba4c9fa4af94e0c68216836567ef/tool/microkit/src/elf.rs#L181

multiple ways to address this; one would be https://lib.rs/crates/deku

Based on that page, we would be replacing 15 lines of code with 57,000 lines of code. We would also be adding at least 4 new dependencies. I looked at the source code for deku, and it is also using unsafe. I don't think there's a way to get this behaviour without using unsafe in Rust (I'm not that familiar with Rust so I could be wrong here).

It seems like we're not removing any unsafe, just making it look like we don't have any unsafe by moving it to a dependency.

Ivan-Velickovic commented 2 weeks ago

There are probably more places to assert that we're working with little-endian machines, but I would much prefer improving the existing code than adding a massive dependency.

wucke13 commented 2 weeks ago

I don't think there's a way to get this behaviour without using unsafe in Rust (I'm not that familiar with Rust so I could be wrong here).

There is, for sure. The question is what runtime cost we might accept for it (IMHO neglectable) and how one wants it done. @nspin suggested the use of zerocopy, which might provide fewer degrees of freedom in the binary format but might be smaller in code size. Do I correctly read your answer as an "no unsafe solution is desired, but not if it involves big dependencies and not if the dependencies use unsafe"?

In any case I still think having a dependency with vetted unsafe is most often preferable than home grown unsafe blocks.

Ivan-Velickovic commented 2 weeks ago

In any case I still think having a dependency with vetted unsafe is most often preferable than home grown unsafe blocks.

In my opinion it depends, in general I trust dependencies less than our own code. We control everything in this repository, we do not control our dependencies.

Do I correctly read your answer as an "no unsafe solution is desired, but not if it involves big dependencies and not if the dependencies use unsafe"?

Sure. Not all unsafe is the same so I guess it depends.

I will admit that I didn't look to closely at these unsafe bits in the tool so the first thing would be to understand under which cases this leads to unsafe behaviour and whether those cases are acceptable.

wucke13 commented 1 week ago

@Ivan-Velickovic I think I addressed most of your nitpicks. Shall we keep this history, or do you want me to squash the bits that belong together? Squashed it.

Ivan-Velickovic commented 1 week ago

I've just pushed a couple changes to get CI passing.

wucke13 commented 1 week ago

I might have force pushed over your changes inadvertently

Ivan-Velickovic commented 1 week ago

I might have force pushed over your changes inadvertently

No worries I'll add back the changes and then merge.

Thanks for making the fixes.