Lib.rs

› DTB › Reviews

These reviews are from Crev, a distributed system for code reviews. To add your review, set up cargo-crev.

The current version of DTB is 0.2.0.

0.1.3 (older version) Rating: Neutral Thoroughness: Medium Understanding: Medium

by HeroicKatora on 2019-10-01

Has fuzzing integration and been scrutinized by a previous review. I've done version 0.1.2 in some detail and the only changes merged since then are bug fix PRs addressing those. Mostly worried about inadvertently introduced bugs in future changes due to awkward interfaces and implicit trust between different components within the code.

For example, code in struct_item.rs relies on the reader providing only 4 byte aligned buffers instead of reasserting that fact as an internal invariant. However this invariant is relied upon for an aligned pointer load later.

Similarly some other C style patterns are prominent. Instead of functions testing an invariant and constructing a result based off of it, boolean return and construction by the caller is used. Example:

      self.assert_enough_struct(offset, desc_size)?;

      let desc_be = unsafe {
          &*((&self.struct_block[offset..]).as_ptr() as *const PropertyDesc)
              as &PropertyDesc
      };

This is fine in its current usage as far as I can tell but it's not very stable with regards to possible future changes.

This also means that manual arithmetic is used for bounds checks which is prone to missed overflow considerations etc.

A remaining antipattern is that of a byte output buffer: StructItem offers reading its value as strings or a u32 list. But instead of an iterator over the backing memory the implementation takes an mutable reference to a byte slice, manually aligns it to fit the output type, casts it, and writes the data types &'_ str and u32. I have not found concrete misbehaviour from this but it seems awkward.

0.1.2 (older version) Rating: Negative Thoroughness: Medium Understanding: Medium

by HeroicKatora on 2019-09-08

Show review…

Liberal use of unsafe and sparse validation of in puts indices and offsets. In principle, the dtb format lends itself well to this use as the file format itself already requires the alignment of many members and takes care to have naturally packed structs–with aligned members but no padding.

It is thus possibly safe to map many parts of an immutable input directly to structs marked as repr(C), which also correctly appears.

However, the unsafe blocks contain only few indications of consideration of their safety. Sometimes alignment checks appear obviously above but most iterators implicitely trust their callers on the alignment of internal buffers. It also seems that not all functions relying on unsafe preconditions are marked unsafe. This applies to internal functions only but may make the crate more brittle than necessary.

Another antipattern is that of a byte output buffer: A StructItem offers reading its value as strings or a u32 list. But instead of an iterator over the backing memory the implementation takes an mutable reference to a byte slice, manually aligns it to fit the output type, casts it, and writes the data types &'_ str and u32. I have not found concrete misbehaviour from this but it seems awkward.

Lib.rs has been able to verify that all files in the crate's tarball, except Cargo.lock, are in the crate's repository. Please note that this check is still in beta, and absence of this confirmation does not mean that the files don't match.

Crates in the crates.io registry are tarball snapshots uploaded by crates' publishers. The registry is not using crates' git repositories, so there is a possibility that published crates have a misleading repository URL, or contain different code from the code in the repository.

To review the actual code of the crate, it's best to use cargo crev open dtb. Alternatively, you can download the tarball of dtb v0.2.0 or view the source online.