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

The current version of string-interner is 0.12.2.

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.