Lib.rs

› StringInterner › Reviews

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

The current version of StringInterner is 0.15.0.

0.7.1 (older version) Rating: Neutral Thoroughness: None Understanding: None

by dpc on 2019-10-12

0.7.1 (older version) Rating: Positive Thoroughness: Medium Understanding: High

by lo48576 on 2019-09-02

I have reviewed the previous version (0.7.0) and sent some patches to make the crate safer.

In this version, I found no security problems and no malicious code.

  • unsafe at line 158 is safe because
    • both InternalStrRefs and strings referred by them are immutable,
    • strings referred by InternalStrRefs won't be dropped until the owning interner is dropped,
    • they have *const str referring inside private Vec<Box<str>> which is owned by the same interner, and
    • InternalStrRefs are not taken or copied to the outside of StringInterner type.
  • unsafe impls at line 248 and 254 is safe as commented in the source.
    • Addresses of strings in Vec<Box<str>> is not changed until the owning interner is dropped.
    • InternalStrRefs and their content pointers are not exposed to the users.
    • Pointers to the strings are read-only.

I think this crate is safe to use.

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

by lo48576 on 2019-10-09

Show review…

EDIT: The found unsoundness is reported as vulnerability (RUSTSEC-2019-0023). This is already fixed in 0.7.1, so you should use 0.7.1 or above.

I've found unsoundness and reported it: https://github.com/Robbepop/string-interner/issues/9. This is use after free, but user cannot access that dangling pointer directly, and invalid write access will not happen. This issue is caused by StringInterner::clone() followed by StringInterner::{get,get_or_intern}, so this would affect many use cases. This is the reason of negative rating.

Except for that issue, there seems no problem.

  • unsafe at line 125 is safe thanks to assert! at the previous line.
  • unsafe at line 165 is safe because
    • InternalStrRefs are immutable,
    • they have *const str referring inside private Vec<Box<str>>, and
    • InternalStrRefs are not taken or copied to the outside of StringInterner type.
    • (It is unsafe that InternalStrRef refers string owned by another interner instance, and it is reported as stated above.)
  • unsafe impls at line 233 and 239 is safe as commented in the source.
    • Addresses of strings in Vec<Box<str>> is not changed until drop.
    • InternalStrRef and its content pointers are not exposed to the users.
    • Pointers to the strings are read-only.

Once the UB bug is fixed, this crate would be safe.

Lib.rs has been able to verify that all files in the crate's tarball are in the crate's repository with a git tag matching the version. 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 string-interner. Alternatively, you can download the tarball of string-interner v0.15.0 or view the source online.