Lind-Project / lind_project

Lind: Secure Lightweight Adaptive Isolation
https://hub.docker.com/r/securesystemslab/lind
Apache License 2.0
23 stars 7 forks source link

Use rust's clippy utility... #373

Open JustinCappos opened 1 month ago

JustinCappos commented 1 month ago

We should be using a linter to help clean up the codebase. Clippy seems to be great, let's use it.

root@9eaaabfc2ea8:/home/test/safeposix-rust# cargo clippy
...
warning: `rustposix` (lib) generated 422 warnings
error: could not compile `rustposix` (lib) due to 13 previous errors; 422 warnings emitted

Right now, we have a long way to go to appease clippy. I am predicting we will find a lot of errors when doing this cleanup!

JustinCappos commented 1 month ago

I've started to look through why clippy isn't happy with rustposix and see the following:

174 of the warnings are for unnecessary return statements. As someone who hasn't really wrapped their head around Rust fully, I prefer the style that clippy complains about over the version it wants us to change our code to use. So I would consider this case low priority to fix. If we want to fix it, clippy will do so automatically for us.

There are a few dozen other warnings that are also likely not a real issue and may be low priority to fix. For example, there are a fair number of warnings about us doing type conversions to the same type. Auto dereferencing also would be done in some cases where we are explicit. Redundant field initialization is another case of this. To me these are examples of situations where we likely do not make the code less clear by being explicit, but then again, maybe the fact we're doing a redundant operation will confuse some readers? Fixing these seems like it isn't the highest priority.

There are another few dozen which are likely efficiency bugs that we could fix. For example, apparently a length comparison to zero is often much less efficient than is_empty().

The remainder of issues noted by clippy are a mix of possible bugs and things that could cause buts. These require deeper understanding of the code and need to be examined manually. For example, there are places where we are doing operations that have no effect, having if statements where the if and else blocks do the same thing, etc. These are likely errors that we do not realize we have in our code. The zero_prefixed_literal warnings are particularly scary! I think a lot of places we use these, the assumption is they are binary or hex values!!!

I don't know how to judge the errors, but there are only 13 of them, so someone should take a closer look. I do think they are much more likely to be safe / correct than the errors above.

I propose we prioritize things in the last category and temporarily turn off clippy's warnings for things in the first two categories like unnecessary return statements.

JustinCappos commented 1 month ago

I've looked more through here. There are a lot of benign things which are likely not a problem but we may want to change for style reasons. There are also a bunch that are mild performance issues, but not security bugs. I see quite a few that should be obviously fixed, but which aren't likely causing us an actual issue.

For things which may be a bigger concern, I've listed them here. I honestly can't tell in these cases, but they seem to have the potential to be an actual problem. Note there are often multiple copies of each of these, but I'm only listing one here. If any are an issue, please let me know and I can potentially give more examples.

error: written amount is not handled
   --> src/interface/file.rs:201:17
    |
201 |                 fobj.write(buf)?;
    |                 ^^^^^^^^^^^^^^^^
    |
    = help: use `Write::write_all` instead, or handle partial writes
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unused_io_amount
    = note: `#[deny(clippy::unused_io_amount)]` on by default

warning: this let-binding has unit value
   --> src/interface/file.rs:269:9
    |
269 |         let _newsize = f.set_len((COUNTMAPSIZE + mapsize) as u64).unwrap();
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f.set_len((COUNTMAPSIZE + mapsize) as u64).unwrap();`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value
    = note: `#[warn(clippy::let_unit_value)]` on by default

    |
187 |     for <item> in slice.iter_mut().take(count) {slice[i] = 0u8;}
    |         ~~~~~~    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: this public function might dereference a raw pointer but is not marked `unsafe`
   --> src/interface/misc.rs:192:55

warning: this lifetime isn't used in the function definition
   --> src/interface/types.rs:352:22
    |
352 | pub fn get_ioctl_int<'a>(ptrunion: IoctlPtrUnion) -> Result<i32, i32> {
    |                      ^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes
    = note: `#[warn(clippy::extra_unused_lifetimes)]` on by default

warning: this `if` has identical blocks
    --> src/safeposix/syscalls/fs_calls.rs:1686:55
     |
1686 |                       if operation & LOCK_NB == LOCK_NB {
     |  _______________________________________________________^
1687 | |                         lock.unlock();
1688 | |                     } else {
     | |_____________________^
     |
note: same as this
    --> src/safeposix/syscalls/fs_calls.rs:1688:28
     |
1688 |                       } else {
     |  ____________________________^
1689 | |                         lock.unlock();
1690 | |                     }
     | |_____________________^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
     = note: `#[warn(clippy::if_same_then_else)]` on by default

warning: unnecessary operation
    --> src/safeposix/syscalls/net_calls.rs:1838:25
     |
1838 |                         epollfdobj.registered_fds.remove(&fd).unwrap().1;
     |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: statement can be reduced to: `epollfdobj.registered_fds.remove(&fd).unwrap();`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
     = note: `#[warn(clippy::unnecessary_operation)]` on by default

warning: redundant closure
  --> src/safeposix/filesystem.rs:26:36
   |
26 |     interface::RustLazyGlobal::new(|| interface::RustHashMap::new());
   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `interface::RustHashMap::new`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
   = note: `#[warn(clippy::redundant_closure)]` on by default

error: read amount is not handled
  --> src/lib_fs_utils.rs:78:9
   |
78 |         hostfile.read(hostslice.as_mut_slice()).unwrap();
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: use `Read::read_exact` instead, or handle partial reads
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unused_io_amount