logo

Rust crates reviews

Cryptographically verifiable, distributed dependency reviews

reviewer: MaulingMonkey

https://lib.rs/MaulingMonkey

$ cargo crev repo fetch url https://github.com/MaulingMonkey/crev-proofs
$ cargo crev id trust 6OZqHXqyUAF57grEY7IVMjRljdd9dgDxiNtr1NF1BdY

repo: https://github.com/MaulingMonkey/crev-proofs

crate version
rating
date
reviewer
thoroughness, understanding
negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-23
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

negative
2019-07-24
MaulingMonkey
none, none

https://64.github.io/actix/https://github.com/actix/actix-web/pull/968#issuecomment-509894555

2020-11-01
MaulingMonkey
advisories:
high
minor

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)

Full Audit

positive
2020-11-01
MaulingMonkey
high, high
advisories:
high
minor

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)

Full Audit

neutral
2020-11-01
MaulingMonkey
high, high
issues:
high
Catch-all traits defining `length` could mess with `array!` to cause UB
advisories:
high

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)

Full Audit

negative
2020-11-01
MaulingMonkey
high, high
issues:
high
core::mem::uninitialized is deprecated / undefined behavior

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)
  • core::mem::uninitialized until 1.0.4 is UB

Full Audit

negative
2020-11-01
MaulingMonkey
high, high
issues:
high
core::mem::uninitialized is deprecated / undefined behavior

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)
  • core::mem::uninitialized until 1.0.4 is UB

Full Audit

negative
2020-11-01
MaulingMonkey
high, high
issues:
high
core::mem::uninitialized is deprecated / undefined behavior

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)
  • core::mem::uninitialized until 1.0.4 is UB

Full Audit

negative
2020-11-01
MaulingMonkey
high, high
issues:
high
core::mem::uninitialized is deprecated / undefined behavior

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)
  • core::mem::uninitialized until 1.0.4 is UB

Full Audit

negative
2020-11-01
MaulingMonkey
high, high
issues:
high
core::mem::uninitialized is deprecated / undefined behavior

Pros:

  • Solid and awesome

Cons:

  • unsafe in macro prevents use with #![forbid(unsafe_code)]
  • Bumps MSRV on patch versions (but so far only for damn good reasons)
  • core::mem::uninitialized until 1.0.4 is UB

Full Audit

neutral
2019-12-22
MaulingMonkey
medium, medium

Stack/value variable length arrays without heap fallback.

Pros:

  • Maybe sound?
  • Better than what you'll write.

Cons:

  • History of unsoundness (0.4.10 and earlier)
  • Disturbing amounts of unsafe

This version switched some slices possibly containing uninit (UB!) to use
pointers instead. This makes encode_utf8 unsafe, sadly.

DiffRatingNotes
.cargo_vcs_info.json+1
.gitignore+1
Cargo.lock+1Rust version bump?
Cargo.toml+1debug [profile.*]
Cargo.toml.orig+1debug [profile.*]
README.rst+1
*.{events,string_data,string_index}0Binary test files, unreviewed
src/array.rs+1Removed #[inline]
src/array_string.rs+1Added fn len, removed #[inline], use ptr instead of slice
src/chars.rs+1encode_utf8 is now sadly unsafe, more test coverage
src/lib.rs+1Inline tweaks, more (correct) ptr use, add as_*_ptr to match Vec (safe/sound)
neutral
2019-09-25
MaulingMonkey
medium, medium

Stack/value variable length arrays without heap fallback.

Pros:

  • Maybe sound?
  • Better than what you'll write.

Cons:

  • History of unsoundness (0.4.10 and earlier)
  • Disturbing amounts of unsafe
DiffRatingNotes
.cargo_vs_info.json+1
.travis.yml+1MSRV bumped to 1.36.0, features tweaked.
Cargo.toml+1feature "serde-1" -> "serde", 2018 edition, drop cruft
Cargo.toml.orig+1
README.rst0"(not yet released)" no longer accurate.
benches/extend.rs+1+black_box
build.rs+1Dropped?
src/array.rs+1Improved safety docs, although could use more explaination of what relies on the invariants. () and bool indexing for 1/2-len arrays.
src/array_string.rs0mem::zeroed -> MaybeUninitCopy::uninitialized. Lots of Copy constraints, one transmute -> from_utf8_unchecked_mut (safer).
src/lib.rs0truncate now unsafe (but sound), new try_extend_from_slice is unsafe (but sound). ArrayVec::extend ZST handling is obtuse, would be unsound in C++, but I believe sound in Rust, maybe?
src/maybe_uninit.rs+1
src/maybe_uninit_nodrop.rs+1Removed
src/maybe_uninit_stable.rs+1Removed
src/range.rs+1Removed
tests/serde.rs+1
tests/tests.rs+1New test cases
neutral
2019-07-28
MaulingMonkey
high, medium

Probably sound as of 0.4.11 on Rust 1.36.0+?
Uses a disturbing amount of unsafe, but aside from uninitialized! use in ArrayVec 1.35 and earlier, it all at least appears to be correct after a careful reading.
Unlike smallvec, this doesn't fall back on the heap.
Better than whatever you'll write rolling your own, at least.

0.4.7 -> 0.4.8: IntoIter implemented Clone, unconcerning
0.4.8 -> 0.4.9: ArrayString initialized to 0, ArrayVec uses nightly MaybeUninit. Unfortunately stable still uses uninitialized!() so this is still negative.
0.4.9 -> 0.4.10: #[repr(C)], -Clone for MaybeUninit. Apparently I missed more possible unsoundness. Unsafe is hard.
0.4.10 -> 0.4.11: ArrayVec should now also be sound in Rust 1.36.0+, probably, maybe.

negative
2019-07-28
MaulingMonkey
high, medium

Prefer 0.4.11 which at least starts using MaybeUninit instead of uninitialized!(), which is fundamentally unsound.
Uses a disturbing amount of unsafe, but aside from uninitialized, it all at least appears to be correct after a careful reading.
Unlike smallvec, this doesn't fall back on the heap.
Better than whatever you'll write rolling your own, at least.

0.4.7 -> 0.4.8: IntoIter implemented Clone, unconcerning
0.4.8 -> 0.4.9: ArrayString initialized to 0, ArrayVec uses nightly MaybeUninit. Unfortunately stable still uses uninitialized!() so this is still negative.
0.4.9 -> 0.4.10: #[repr(C)], -Clone for MaybeUninit. Apparently I missed more possible unsoundness. Unsafe is hard.

negative
2019-07-28
MaulingMonkey
high, medium

Prefer 0.4.11 which at least starts using MaybeUninit instead of uninitialized!(), which is fundamentally unsound.
Uses a disturbing amount of unsafe, but aside from uninitialized, it all at least appears to be correct after a careful reading.
Unlike smallvec, this doesn't fall back on the heap.
Better than whatever you'll write rolling your own, at least.

0.4.7 -> 0.4.8: IntoIter implemented Clone, unconcerning
0.4.8 -> 0.4.9: ArrayString initialized to 0, ArrayVec uses nightly MaybeUninit. Unfortunately stable still uses uninitialized!() so this is still negative.

negative
2019-07-28
MaulingMonkey
high, medium

Prefer 0.4.11 which at least starts using MaybeUninit instead of uninitialized!(), which is fundamentally unsound.
Uses a disturbing amount of unsafe, but aside from uninitialized, it all at least appears to be correct after a careful reading.
Unlike smallvec, this doesn't fall back on the heap.
Better than whatever you'll write rolling your own, at least.

See 0.4.7 for base review. Diffed 0.4.7 -> 0.4.8, no concerning changes.

negative
2019-07-28
MaulingMonkey
high, medium

Prefer 0.4.11 which at least starts using MaybeUninit instead of uninitialized!(), which is fundamentally unsound.
Uses a disturbing amount of unsafe, but aside from uninitialized, it all at least appears to be correct after a careful reading.
Unlike smallvec, this doesn't fall back on the heap.
Better than whatever you'll write rolling your own, at least.

Detail

FileRatingNotes
benches/arraystring.rs+1
benches/extend.rs+1
src/array_string.rs0lots of unsafe, but I think sound
src/array.rs0fix_array_impl! hides unsafe, but not misused nor public
src/char.rs+1Relied upon for soundness... thoroughly checked against https://en.wikipedia.org/wiki/UTF-8
src/errors.rs+1
src/lib.rs0lots of unsafe, but I think sound
src/range.rs+1
tests/serde.rs+1
tests/tests.rs+1
.gitignore+1
.travis.yml+1
Cargo.toml+1
Cargo.toml.orig+1
custom.css+1
LICENSE-APACHE+1
LICENSE-MIT+1
README.rst+1
OtherRatingNotes
unsafe-1Overused
fs+1Unused
docs+1
tests0Good coverage... not seeing any fuzz testing for all this unsafe though.

src/array_string.rs

OK

LineNotes
56unsafe - new_array ~ uninitialized, Array is an unsafe trait though so only implement it if this is sound...?
95No CapacityError? Inconsistent vs from...
160unsafe { ... } - looks correct
213unsafe { ... } - looks correct
216could be a slice copy instead
245unsafe { ... } - looks correct
271unsafe { ... } - looks correct
307unsafe { ... } - looks correct
318unsafe { ... } - looks correct
331unsafe fn - decent docs, looks correct, should be more explicit about uninitialized though
342unsafe fn - needs better docs, but looks correct
351unsafe { ... } - looks correct
361unsafe { ... } - scary transmute, but just from &mut [u8] to &mut str. stdlib from_utf8_unchecked does equivalent pointer casts

src/array.rs

LineNotes
80Aieee!
132unsafe { ... } - not sure this is sound for bools etc.
214unsafe { ... } - looks correct
246unsafe fn - exactly as spceified
306unsafe { ... } - looks correct
340unsafe { ... } - looks correct
511unsafe fn - exactly as specified
552unsafe { ... } - scary as heck... but Drain should keep self borrowed long enough, at least.
575unsafe { ... } - looks correct
604unsafe { ... } - looks correct
614unsafe { ... } - looks correct
707unsafe { ... } - looks correct. Size could be reduced, relies on IntoIter's custom drop not dropping copied elements due to the index increment to avoid double drops.
724unsafe { ... } - looks correct. Size could be reduced, relies on IntoIter's custom drop not dropping copied elements due to the length decrement to avoid double drops.
740unsafe { ... } - looks correct. Implements the aforementioned IntoIter custom drop.
764unsafe Sync - I believe this is OK.
765unsafe Send - I believe this is OK.
775unsafe { ... } - looks correct. Relies on set_len already being truncated to avoid double drops.
793unsafe { ... } - looks correct. Relies on set_len already being truncated to avoid double drops.
809necessary to aovid memory leaks
812unsafe { ... } - looks correct.
851unsafe { ... } - looks correct.
1008unsafe { ... } - looks correct.
negative
2019-07-29
MaulingMonkey
none, high

UNSOUND: https://github.com/tomprogrammer/rust-ascii/issues/64 (I didn't cath this one)
unsound? https://github.com/tomprogrammer/rust-ascii/issues/65

Lots of repeated unsafe.
No debug_assert! s for uncheck codepaths.
Unsound test code assumes gen_range meets it's safe API contract.
Not fully reviewed.

Detail

FileRatingNotes
src/serialization/ascii_char.rs+1thoroughness: low, understanding: high throughout
src/serialization/ascii_str.rs+1
src/serialization/ascii_string.rs+1
src/serialization/mod.rs+1
src/ascii_char.rs0unsound test code?
src/ascii_str.rs-1UNSOUND - missing #[repr(transparent)]` !
src/ascii_string.rsN/AUnreviewed
src/free_functions.rsN/A
src/lib.rsN/A
.gitignoreN/A
.travis.ymlN/A
Cargo.toml+1
Cargo.toml.orig+1
LICENSE-APACHEN/A
LICENSE_MITN/A
README.mdN/A
RELEASES.mdN/A
tests.rsN/A
OtherRatingNotes
unsafe-1UNSOUND, disappointing lack of debug_assert!s.
fs+1?Not present?
io+1?Not present?
docs+1?
tests+1?

src/ascii_char.rs

LineNotes
22This must contain every value between 0..=127 for soundness guarantees bellow.
476unsafe { ... } - looks sound. case 1 handles 32..=126, case 2 handles 127, case 3 handles 0..=31. Not wild about this impl but looks valid. See https://en.wikipedia.org/wiki/Control_Pictures
498unsafe { ... } - looks sound. 'a' > 'A'
509unsafe { ... } - looks sound.
548unsafe { ... } - looks sound. Duplicate logic, annoyingly.
557unsafe { ... } - looks sound. Duplicate logic, annoyingly.
659unsafe fn - looks good.
670unsafe fn - disappointing lack of debug_assert!
678unsafe { ... } - looks sound.
686unsafe fn - disappointing lack of debug_assert!. transmute from u8 to #[repr(u8)] enum... I believe that's sound.
694unsafe { ... } - looks sound.
702unsafe fn - looks sound.
714UNSOUND TEST CODE? no guarantee generic Gen actually generates within range. Gated behind "quickcheck" feature.
735UNSOUND TEST CODE? no guarantee generic Gen actually generates within range. Gated behind "quickcheck" feature.

src/ascii_str.rs

LineNotes
116unsafe fn - looks good.
352UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
359UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
367UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
384scary transmuting impl_into! macro, audit all uses carefully
390UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
397UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
405UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
410I believed these invokes would be sound if AsciiStr was #[repr(transparent)], but https://github.com/tomprogrammer/rust-ascii/issues/64 proved me wrong.
668unsafe fn - looks good.
676unsafe fn - looks good.
689unsafe fn - looks good.
701unsafe fn - looks good.
713unsafe fn - looks good.
724unsafe fn - looks good.
734unsafe fn - looks good.
746unsafe fn - looks good.
756unsafe fn - looks good.
764unsafe { ... } - looks sound.
768unsafe fn - disappointing lack of debug_assert!.
777unsafe { ... } - looks sound.
781unsafe fn - disappointing lack of debug_assert!.
793unsafe fn - looks good.
800unsafe { ... } - looks sound.
804unsafe fn - disappointing lack of debug_assert!.
818unsafe fn - looks good.
negative
2019-07-29
MaulingMonkey
none, high
issues:
high
Definite soundness issue
issues:
high
Probable soundness issue

UNSOUND: https://github.com/tomprogrammer/rust-ascii/issues/64 (I didn't catch this one)
unsound? https://github.com/tomprogrammer/rust-ascii/issues/65

Lots of repeated unsafe.
No debug_assert! s for uncheck codepaths.
Unsound test code assumes gen_range meets it's safe API contract.
Not fully reviewed.

Detail

FileRatingNotes
src/serialization/ascii_char.rs+1thoroughness: low, understanding: high throughout
src/serialization/ascii_str.rs+1
src/serialization/ascii_string.rs+1
src/serialization/mod.rs+1
src/ascii_char.rs0unsound test code?
src/ascii_str.rs-1UNSOUND - missing #[repr(transparent)]` !
src/ascii_string.rsN/AUnreviewed
src/free_functions.rsN/A
src/lib.rsN/A
.gitignoreN/A
.travis.ymlN/A
Cargo.toml+1
Cargo.toml.orig+1
LICENSE-APACHEN/A
LICENSE_MITN/A
README.mdN/A
RELEASES.mdN/A
tests.rsN/A
OtherRatingNotes
unsafe-1UNSOUND, disappointing lack of debug_assert!s.
fs+1?Not present?
io+1?Not present?
docs+1?
tests+1?

src/ascii_char.rs

LineNotes
22This must contain every value between 0..=127 for soundness guarantees bellow.
476unsafe { ... } - looks sound. case 1 handles 32..=126, case 2 handles 127, case 3 handles 0..=31. Not wild about this impl but looks valid. See https://en.wikipedia.org/wiki/Control_Pictures
498unsafe { ... } - looks sound. 'a' > 'A'
509unsafe { ... } - looks sound.
548unsafe { ... } - looks sound. Duplicate logic, annoyingly.
557unsafe { ... } - looks sound. Duplicate logic, annoyingly.
659unsafe fn - looks good.
670unsafe fn - disappointing lack of debug_assert!
678unsafe { ... } - looks sound.
686unsafe fn - disappointing lack of debug_assert!. transmute from u8 to #[repr(u8)] enum... I believe that's sound.
694unsafe { ... } - looks sound.
702unsafe fn - looks sound.
714UNSOUND TEST CODE? no guarantee generic Gen actually generates within range. Gated behind "quickcheck" feature.
735UNSOUND TEST CODE? no guarantee generic Gen actually generates within range. Gated behind "quickcheck" feature.

src/ascii_str.rs

LineNotes
116unsafe fn - looks good.
352UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
359UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
367UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
384scary transmuting impl_into! macro, audit all uses carefully
390UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
397UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
405UNSOUND unsafe { ... } - AsciiStr is not #[repr(transparent)] !
410I believed these invokes would be sound if AsciiStr was #[repr(transparent)], but https://github.com/tomprogrammer/rust-ascii/issues/64 proved me wrong.
668unsafe fn - looks good.
676unsafe fn - looks good.
689unsafe fn - looks good.
701unsafe fn - looks good.
713unsafe fn - looks good.
724unsafe fn - looks good.
734unsafe fn - looks good.
746unsafe fn - looks good.
756unsafe fn - looks good.
764unsafe { ... } - looks sound.
768unsafe fn - disappointing lack of debug_assert!.
777unsafe { ... } - looks sound.
781unsafe fn - disappointing lack of debug_assert!.
793unsafe fn - looks good.
800unsafe { ... } - looks sound.
804unsafe fn - disappointing lack of debug_assert!.
818unsafe fn - looks good.
neutral
2020-03-19
MaulingMonkey
medium, medium

Not suitable for UGC - memory exhaustion and panic concerns.
https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/ase.md#0.1.3

neutral
2020-03-19
MaulingMonkey
medium, medium

Not suitable for UGC - memory exhaustion and panic concerns.
https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/ase.md#0.1.1

neutral
2020-03-19
MaulingMonkey
medium, medium

Not suitable for UGC - memory exhaustion and panic concerns.
https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/ase.md#0.1.0

positive
2020-10-11
MaulingMonkey
medium, high

[Full Audit]
Parses JSON aseprite exports

Pros:

  • Looks fairly exhaustive, self contained
  • No direct disk I/O

Cons:

  • Exahustively matchable structs make most version bumps technically breaking changes
  • Requires non-default export options (fixed in an unmerged PR)
  • Enum deserialization is brittle when it comes to new options
  • Limited maintainence
positive
2020-10-11
MaulingMonkey
medium, high

[Full Audit]
Parses JSON aseprite exports

Pros:

  • Looks fairly exhaustive, self contained
  • No direct disk I/O

Cons:

  • Exahustively matchable structs make most version bumps technically breaking changes
  • Requires non-default export options (fixed in an unmerged PR)
  • Enum deserialization is brittle when it comes to new options
  • Limited maintainence
negative
2020-10-11
MaulingMonkey
medium, high

Private fields make this unusable until 0.1.2

negative
2020-10-11
MaulingMonkey
medium, high

Private fields make this unusable until 0.1.2

positive
2019-12-22
MaulingMonkey
medium, medium

LGTM, starts using RUSTFLAGS

positive
2019-09-03
MaulingMonkey
medium, medium

0.1.6: LGTM
0.1.5: No unsafe code, minor safe-looking file I/O

positive
2019-07-23
MaulingMonkey
medium, medium

No unsafe code, minor safe-looking file I/O

positive
2020-09-11
MaulingMonkey
high, high
alternative:
More generalized, less kludgy. However, File::read_at/write_at require &mut self on windows by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • read_at(&self, ...)

Cons:

  • BufOffsetReader is a bit kludgy - u64 as usize casts should probably Err instead
  • Implementing traits directly on File makes for inconsistent seek behavior when used by reference

Full Audit

positive
2020-09-11
MaulingMonkey
high, high
alternative:
More generalized, less kludgy. However, File::read_at/write_at require &mut self on windows by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • read_at(&self, ...)

Cons:

  • BufOffsetReader is a bit kludgy - u64 as usize casts should probably Err instead
  • Implementing traits directly on File makes for inconsistent seek behavior when used by reference

Full Audit

positive
2020-09-11
MaulingMonkey
high, high
alternative:
More generalized, less kludgy. However, File::read_at/write_at require &mut self on windows by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • read_at(&self, ...)

Cons:

  • BufOffsetReader is a bit kludgy - u64 as usize casts should probably Err instead
  • Implementing traits directly on File makes for inconsistent seek behavior when used by reference

Full Audit

positive
2020-09-11
MaulingMonkey
high, high
alternative:
More generalized, less kludgy. However, File::read_at/write_at require &mut self on windows by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • read_at(&self, ...)

Cons:

  • BufOffsetReader is a bit kludgy - u64 as usize casts should probably Err instead
  • Implementing traits directly on File makes for inconsistent seek behavior when used by reference

Full Audit

positive
2020-09-11
MaulingMonkey
high, high
alternative:
More generalized, less kludgy. However, File::read_at/write_at require &mut self on windows by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • read_at(&self, ...)

Cons:

  • BufOffsetReader is a bit kludgy - u64 as usize casts should probably Err instead
  • Implementing traits directly on File makes for inconsistent seek behavior when used by reference

Full Audit

positive
2020-09-11
MaulingMonkey
high, high
alternative:
More generalized, less kludgy. However, File::read_at/write_at require &mut self on windows by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • read_at(&self, ...)

Cons:

  • BufOffsetReader is a bit kludgy - u64 as usize casts should probably Err instead
  • Implementing traits directly on File makes for inconsistent seek behavior when used by reference

Full Audit

positive
2020-09-11
MaulingMonkey
high, high
alternative:
More generalized, less kludgy. However, File::read_at/write_at require &mut self on windows by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • read_at(&self, ...)

Cons:

  • BufOffsetReader is a bit kludgy - u64 as usize casts should probably Err instead
  • Implementing traits directly on File makes for inconsistent seek behavior when used by reference

Full Audit

positive
2019-07-23
MaulingMonkey
high, high

I wrote it, minimal 'unsafe' for FFI, well tested.

negative
2020-08-27
MaulingMonkey
low, medium

See Full Audit

  • MSRV policy!
  • Basic swizzling/endian stuff
  • You didn't have to write it
  • Excessive and distributed unsafe in serialization related code is hard to audit and makes me nervous
  • History of unsoundness. Most crate versions have alignment related bugs, and 0.2.x had overflow issues which regressed in 0.3.x.
  • Very limited functionality
2020-08-27
MaulingMonkey
advisories:
medium
major

Part of my byteorder audit

2020-08-27
MaulingMonkey
advisories:
medium

Part of my byteorder audit

2020-08-27
MaulingMonkey
advisories:
high
major

Part of my byteorder audit

2020-08-27
MaulingMonkey
advisories:
high
major

Part of my byteorder audit

positive
2019-07-23
MaulingMonkey
none, none

https://github.com/rust-lang/cargo - I blindly trust rust

negative
2019-07-24
MaulingMonkey
low, medium

A mixed bag.
On the one hand, the core tool found in src seems fine, solid fundamentals.
On the other hand, the glue code is full chock full of unsafe, some of it unsound.
Worse, the FFI includes things that varies from android SDK to SDK from the loops of things.

injected-glue\ffi.rs: -1
Should be replaced with bindgen generated header? Also, changes between SDKs, so really really really should not be simply hardcoded...!
Ref: https://android.googlesource.com/platform/development/+/4948c163663ecc343c97e4c2a2139234f1d3273f/ndk/sources/android/native_app_glue/android_native_app_glue.h
Ref: https://chromium.googlesource.com/android_tools/+/7fc902d157a9aed7a2b68adc9c69181b3a43cd58/ndk/sources/android/native_app_glue/android_native_app_glue.h
45 LOOPER_ID_INPUT: Wrong? (should be 2?: https://chromium.googlesource.com/android_tools/+/7fc902d157a9aed7a2b68adc9c69181b3a43cd58/ndk/sources/android/native_app_glue/android_native_app_glue.h#204)
LOOPER_ID_EVENT? Different name? https://android.googlesource.com/platform/development/+/4948c163663ecc343c97e4c2a2139234f1d3273f/ndk/sources/android/native_app_glue/android_native_app_glue.h
46 LOOPER_ID_USER: Wrong? (should be 3?: https://chromium.googlesource.com/android_tools/+/7fc902d157a9aed7a2b68adc9c69181b3a43cd58/ndk/sources/android/native_app_glue/android_native_app_glue.h#209)
65+ Skimmed only

injected-glue\lib.rs: -1
92 static mut ANDROID_APP should be UnsafeCell
164 static mut G_MAINTHREAD_BOXED should be UnsafeCell, probable undefined behavior / race conditions
172 scary transmutes
182 Unsound when combined with
191 'safe' access
196 android_main2 is UNSOUND - dereferencing pointers, setting globals, etc. without being marked unsafe.
197+ Skimmed only
340: giant unsafe block
347: uninitialized

src\ops\build.rs: +1
75 Medium: I suspect this won't work on windows as it doesn't use .cmd scripts for the compiler/linker?
131 Medium: injected_glue_lib cmd.exec()?; followed by cmd.exec_with_output()?; (134)- does the glue get double compiled for no good reason?
218 Medium: unreachable! abused where panic! probably should be?
423 Minor: Hardcodes a lot of assumptions. Good foundation though.

src\ops\install.rs: +1
Medium: No way to specify device
src\ops\mod.rs: +1
src\ops\run.rs: +1
Medium: No way to specify device

src\config.rs: +1
Minor: Could use more OsString
Minor: Non-optional opengles_version_*
src\main.rs: +1

Cargo.toml: +1
Cargo.toml.orig: +1
glue_obj.rs: Sketchy looking casts and main handling... but might be right
linker.rs: Strange parse_arguments loop style... but safe, looks sane.

positive
2019-09-17
MaulingMonkey
low, medium

0.4.16: Trivial version bumps.
0.4.15: Trivial version bumps.
0.4.14: Trivial version bumps, mass reformatting.
0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11: Nice and solid looking code. 100% safe code.

Reviewed:
src\cli.rs: +1
src\main.rs: +1
151 run_lldb
Nonterminating loops are problematic, but I think the side effects here should make this work OK?
(see https://github.com/rust-lang/rust/issues/28728 )
Cargo.toml: +1
Cargo.toml.orig: +1
13 readme path references unpackaged readme

TIL:
Using traits to extend clap::App so flags can be reused per-subcommand is interesting.

positive
2019-09-17
MaulingMonkey
low, medium

0.4.15: Trivial version bumps.
0.4.14: Trivial version bumps, mass reformatting.
0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11: Nice and solid looking code. 100% safe code.

Reviewed:
src\cli.rs: +1
src\main.rs: +1
151 run_lldb
Nonterminating loops are problematic, but I think the side effects here should make this work OK?
(see https://github.com/rust-lang/rust/issues/28728 )
Cargo.toml: +1
Cargo.toml.orig: +1
13 readme path references unpackaged readme

TIL:
Using traits to extend clap::App so flags can be reused per-subcommand is interesting.

positive
2019-09-17
MaulingMonkey
low, medium

0.4.14: Trivial version bumps, mass reformatting.
0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11: Nice and solid looking code. 100% safe code.

Reviewed:
src\cli.rs: +1
src\main.rs: +1
151 run_lldb
Nonterminating loops are problematic, but I think the side effects here should make this work OK?
(see https://github.com/rust-lang/rust/issues/28728 )
Cargo.toml: +1
Cargo.toml.orig: +1
13 readme path references unpackaged readme

TIL:
Using traits to extend clap::App so flags can be reused per-subcommand is interesting.

positive
2019-09-09
MaulingMonkey
low, medium

0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11: Nice and solid looking code. 100% safe code.

Reviewed:
src\cli.rs: +1
src\main.rs: +1
151 run_lldb
Nonterminating loops are problematic, but I think the side effects here should make this work OK?
(see https://github.com/rust-lang/rust/issues/28728 )
Cargo.toml: +1
Cargo.toml.orig: +1
13 readme path references unpackaged readme

TIL:
Using traits to extend clap::App so flags can be reused per-subcommand is interesting.

positive
2019-09-03
MaulingMonkey
low, medium

0.4.12: Trivial version bumps.
0.4.11: Nice and solid looking code. 100% safe code.

Reviewed:
src\cli.rs: +1
src\main.rs: +1
151 run_lldb
Nonterminating loops are problematic, but I think the side effects here should make this work OK?
(see https://github.com/rust-lang/rust/issues/28728 )
Cargo.toml: +1
Cargo.toml.orig: +1
13 readme path references unpackaged readme

TIL:
Using traits to extend clap::App so flags can be reused per-subcommand is interesting.

positive
2019-07-26
MaulingMonkey
low, medium

Nice and solid looking code. 100% safe code.

Reviewed:
src\cli.rs: +1
src\main.rs: +1
151 run_lldb
Nonterminating loops are problematic, but I think the side effects here should make this work OK?
(see https://github.com/rust-lang/rust/issues/28728 )
Cargo.toml: +1
Cargo.toml.orig: +1
13 readme path references unpackaged readme

TIL:
Using traits to extend clap::App so flags can be reused per-subcommand is interesting.

neutral
2019-09-24
MaulingMonkey
low, medium

Add/remove/update Cargo.toml dependencies from the command line.

Pros:

  • Safe, probably works

Cons:

  • Lots of code
  • Lots of dependencies, some of which I haven't fully audited.
  • License ambiguity (is this MIT or Apache-2.0/MIT licensed?)

0.4.0

crev
thoroughnesslow
understandingmedium
ratingneutral
DiffRatingNotes
Cargo.lock+1Added since 0.3.3, enabling frozen installs. Approx 200 indirect deps.
Cargo.toml+1Version bumps
Cargo.toml.orig+1Version bumps
README.md+1Mentions new --sort option
appveyor.yml+1Disables gnu targets
src/bin/add/args.rs+1New --sort and --offline options
src/bin/add/main.rs+1
src/bin/upgrade/main.rs+1
src/errors.rs+1
src/fetch.rs+1
src/lib.rs+1
src/manifest.rs+1
src/registry.rs+1
tests/cargo-add.rs+1
tests/fixtures/add/Cargo.toml.unsorted+1
neutral
2019-09-23
MaulingMonkey
low, medium

Add/remove/update Cargo.toml dependencies from the command line.

Pros:

  • Safe, probably works

Cons:

  • Lots of code
  • Lots of dependencies, some of which I haven't fully audited.
  • No Cargo.lock so not installable with --frozen
  • License ambiguity (is this MIT or Apache-2.0/MIT licensed?)

0.3.3

crev
thoroughnesslow
understandingmedium
ratingneutral
FileRatingNotes
src/bin/add/args.rs+1
src/bin/add/main.rs+1
src/bin/add/manifest_test.rs+1
src/bin/rm/main.rs+1
src/bin/upgrade/main.rs+1
src/crate_name.rs-1.contains(url) seems wrong
src/dependency.rs-1No branch support for dependencies?
src/errors.rs+1
src/fetch.rs+1
src/lib.rs+1
src/manifest.rs0find/search duplicate some of cargo metadata's effort I believe
tests/fixtures/add/local/Cargo.toml.sample+1
tests/fixtures/add/Cargo.toml.sample+1
tests/fixtures/manifest-invalid/Cargo.toml.sample+1
tests/fixtures/rm/Cargo.toml.sample+1
tests/fixtures/upgrade/Cargo.toml.invalid+1
tests/fixtures/upgrade/Cargo.toml.source+1
tests/fixtures/upgrade/Cargo.toml.target+1
tests/cargo-add.rs0191: Duplicate assert!s for no reason?
tests/cargo-rm.rs+1Tests are admittedly a bit brittle
tests/cargo-upgrade.rs+1
tests/test_manifest.rs+1
tests/utils.rs017: Pointless clone, beware execute_* passes to exec.
.cargo_vcs_info.json+1
.cargo-ok+1
.editorconfig+1
.gitignore+1
.travis.yml0rustfmt, clippy, travis-cargo, libcurl4-openssl-dev, libelf-dev, libdw-dev, coveralls
appveyor.yml+1
bors.toml+1
Cargo.toml0Apache-2.0/MIT. That's a lot of deps.
Cargo.toml.orig0^
Cargo.lock-1N/A, would nice to be able to --frozen(?) to install fully audited bins
CONTRIBUTING.md+1
LICENSE0Just MIT listed here, Cargo.toml references Apache-2.0/MIT.
README.md0"Apache-2.0/MIT" could be clearer in a Readme.
rustfmt.toml0Empty file
OtherRatingNotes
unsafe+1None, warn if introduced
miriN/ANot bothering with
fs0Manifest related, looks safe?
io0Manifest related
docs+1
tests+1

TIL

  • crates.io API string format
  • Refresher on format! placeholders
format!(
    "{host}/api/v1/crates/{crate_name}",
    host = REGISTRY_HOST,
    crate_name = crate_name
);
positive
2019-07-24
MaulingMonkey
low, medium

Pretty trivial, not even sure if it'll work right on windows to be worth using.

positive
2019-12-22
MaulingMonkey
medium, medium

Parse cargo metadata and cargo build --message-format=json output.

Pros:

  • Way better than parsing it yourself
  • Safe code

Cons:

  • If you're feeling particularly paranoid, cargo metadata could be passed bad
    feature names (see 0.8.2 review for details)

0.9.1

crev
thoroughnessmedium
understandingmedium
ratingpositive
DiffRatingNotes
.cargo_vcs_info.json+1
Cargo.toml+1
Cargo.toml.orig+1
src/dependency.rs+1
src/lib.rs+1
src/messages.rs+1
tests/test_samples.rs+1
positive
2019-12-22
MaulingMonkey
medium, medium

Parse cargo metadata and cargo build --message-format=json output.

0.9.0

DiffRatingNotes
.cargo_vcs_info.json+1
Cargo.toml+1
Cargo.toml.orig+1
src/errors.rs+1Added Error::NoJson
src/lib.rs0Various safe but breaking code changes
src/messages.rs+1
tests/selftest.rs+1
tests/test_samples.rs+1

0.8.2

FileRatingNotes
src/dependency.rs+1
src/diagnostic.rs+1
src/errors.rs+1
src/lib.rs0MetadataCommand makes me slightly paranoid
src/messages.rs+1
tests/selftest.rs+1
tests/test_samples.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+11.32.0 MSRV
Cargo.toml+1
Cargo.toml.orig+1
LICENSE-MIT+1
README.md+1
OtherRatingNotes
unsafe+1None
fs+1None
io0Can invoke cargo metadata. Looks sane, but if passed malicious feature names etc...
docs+1
tests+1

src/lib.rs

LineWhatNotes
495execshell access, and I'm paranoid about shell param escaping...
500execshell access, and I'm paranoid about shell param escaping...
positive
2019-09-07
MaulingMonkey
medium, medium

Parse cargo metadata and cargo build --message-format=json output.

0.8.2

FileRatingNotes
src/dependency.rs+1
src/diagnostic.rs+1
src/errors.rs+1
src/lib.rs0MetadataCommand makes me slightly paranoid
src/messages.rs+1
tests/selftest.rs+1
tests/test_samples.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+11.32.0 MSRV
Cargo.toml+1
Cargo.toml.orig+1
LICENSE-MIT+1
README.md+1
OtherRatingNotes
unsafe+1None
fs+1None
io0Can invoke cargo metadata. Looks sane, but if passed malicious feature names etc...
docs+1
tests+1

src/lib.rs

LineWhatNotes
495execshell access, and I'm paranoid about shell param escaping...
500execshell access, and I'm paranoid about shell param escaping...
positive
2019-09-25
MaulingMonkey
low, low

Macro black magic... I trust the author and what I do understand looks good.

positive
2019-07-31
MaulingMonkey
low, low

Macro black magic... I trust the author and what I do understand looks good.

neutral
2019-07-23
MaulingMonkey
low, medium

Interesting idea. Lots of unsafe! Supposedly auto-generated.
I have at least read the code enough (including explicitly looking at each use of unsafe)
to believe there are no obvious funny business nor proper soundness issues. That said:

Concerns (rating: neutral):
Lots of concerning use of unsafe { ... } + core::mem::uninitialized()... but only in #[test] code.
#[allow(improper_ctypes)] for extern "C" block. Sounds like UB-bait.
Punts all safety concerns to the user.

Concerns (thoroughness: low):
I have not properly vetted fn/struct/type signatures against a proper reference.

positive
2020-06-18
MaulingMonkey
medium, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/codepage-437.md#0.1.0

negative
2019-09-25
MaulingMonkey
medium, medium
issues:
medium
Getting the cursor pos in ANSI mode can drop stdin data
issues:
high
Winapi save/restore cursor invokes undefined behavior: `static mut SAVED_CURSOR_POS` not guarded
issues:
high
Unsound access of possibly invalid `HANDLE`s

Pros:

  • Cross platform

Cons:

  • Soundness issues
  • Data races
FileRatingNotes
.github/CODEOWNERS+1
docs/CONTRIBUTING.md+1
src/cursor/ansi_cursor.rs+1
src/cursor/cursor.rs+1
src/cursor/winapi_cursor.rs+1
src/sys/unix.rs-1[#3] 45: Getting the cursor pos can drop stdin data
src/sys/winapi.rs-1[#4, #5] Multiple soundness issues
src/cursor.rs+1
src/lib.rs+1
src/sys.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+1
Cargo.toml+1MIT, dep: winapi
Cargo.toml.orig+1MIT, dep: winapi
CHANGELOG.md+1
LICENSE+1MIT
README.md+1
OtherRatingNotes
unsafe-1Soundness issues
fs+1None
io0Drops stdin
docs+1
tests+1

src/sys/winapi.rs

LineWhatNotes
26unsafe mut SAVED_CURSOR_POS-1, [#4] Access to static mut is unguarded! Undefined behavior! Unsound!
68fn Cursor::goto0, [#5] unsafe { ... } - would be valid if screen buffer handle was guaranteed valid
86fn Cursor::set_visibility0, [#5] unsafe { ... } - would be valid if screen buffer handle was guaranteed valid
101fn Cursor::restore_cursor_pos-1, [#4] Access to static mut is unguarded! Undefined behavior! Unsound!
114fn Cursor::save_cursor_pos-1, [#4] Access to static mut is unguarded! Undefined behavior! Unsound!
121impl From<!-- raw HTML omitted --> for Cusror??, [#5] Not sure if Handle is guaranteed to be valid
129impl From<!-- raw HTML omitted --> for Cursor-1, [#5] no guarantee HANDLE is valid, unsound!
negative
2019-09-26
MaulingMonkey
medium, medium
issues:
medium
Getting the cursor pos in ANSI mode can drop stdin data
issues:
high
Winapi save/restore cursor invokes undefined behavior: `static mut SAVED_CURSOR_POS` not guarded
issues:
high
Unsound access of possibly invalid `HANDLE`s

Pros:

  • Cross platform

Cons:

  • Soundness issues
  • Data races
FileRatingNotes
.github/CODEOWNERS+1
docs/CONTRIBUTING.md+1
src/cursor/ansi_cursor.rs+1
src/cursor/cursor.rs+1
src/cursor/winapi_cursor.rs+1
src/sys/unix.rs-1[#199] 45: Getting the cursor pos can drop stdin data
src/sys/winapi.rs-1[#245, #252] Multiple soundness issues
src/cursor.rs+1
src/lib.rs+1
src/sys.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+1
Cargo.toml+1MIT, dep: winapi
Cargo.toml.orig+1MIT, dep: winapi
CHANGELOG.md+1
LICENSE+1MIT
README.md+1
OtherRatingNotes
unsafe-1Soundness issues
fs+1None
io0Drops stdin
docs+1
tests+1

src/sys/winapi.rs

LineWhatNotes
26unsafe mut SAVED_CURSOR_POS-1, [#245] Access to static mut is unguarded! Undefined behavior! Unsound!
68fn Cursor::goto0, [#252] unsafe { ... } - would be valid if screen buffer handle was guaranteed valid
86fn Cursor::set_visibility0, [#252] unsafe { ... } - would be valid if screen buffer handle was guaranteed valid
101fn Cursor::restore_cursor_pos-1, [#245] Access to static mut is unguarded! Undefined behavior! Unsound!
114fn Cursor::save_cursor_pos-1, [#245] Access to static mut is unguarded! Undefined behavior! Unsound!
121impl From<!-- raw HTML omitted --> for Cusror??, [#252] Not sure if Handle is guaranteed to be valid
129impl From<!-- raw HTML omitted --> for Cursor-1, [#252] no guarantee HANDLE is valid, unsound!
negative
2019-09-25
MaulingMonkey
medium, medium
issues:
high
Unguarded access of static mut ORIG_MODE is unsound

Pros:

  • Handles console input

Cons:

  • Soundness issues
  • Not browser compatible
FileRatingNotes
.github/CODEOWNERS+1
docs/CONTRIBUTING.md+1
src/input/input.rs+1
src/input/unix_input.rs-1Parsing looks off, panicy internals
src/input/windows_input.rs-1Unsound [#245], very strange keyboard handling.
src/sys/unix.rs0Mishandles read == 0?
src/input.rs+1
src/lib.rs+1
src/sys.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
travis.yml+1
Cargo.toml+1MIT, winapi, libc, optional serde
Cargo.toml.orig+1MIT, winapi, libc, optional serde
CHANGELOG.md+1
LICENSE+1MIT
README.md+1MIT
OtherRatingNotes
unsafe-1Unsound
fs+1/dev/tty access, but that's expected
io+1Sound... probably
docs+1
tests0Admittedly hard to unit test

src/input/unix_input.rs

LineWhatNotes
198fn SyncReader::next-1, Disambiguating ESC like this seems super sketchy/brittle.
261fn parse_event-1, This is more like what ESC parsing should look like...?
269fn parse_event-1, \r\n -> Enter Enter? Seems wrong...
312fn parse_csi-1, .unwrap Panic city
EOF

src/input/windows_input.rs

LineWhatNotes
42static mut ORIG_MODE-1, More unsound access [[#2]]
47WindowsInput::read_char+1, unsafe { ... } - willing to assume _getwche is sound.
110fn WindowsInput::enable_mouse_mode-1, unsafe { ... } - unsound access of ORIG_MODE! [#245]
116fn WindowsInput::disable_mouse_mode-1, unsafe { ... } - unsound access of ORIG_MODE! [#245]
225fn read_single_event0, unsafe { ... } - KeyEventRecord::from(*input.event.KeyEvent()) is probably sound/safe?
228fn read_single_event0, unsafe { ... } - MouseEvent::from(*input.event.MouseEvent()) is probably sound/safe?
249fn read_input_events0, unsafe { ... } - KeyEventRecord::from(*input.event.KeyEvent()) is probably sound/safe?
256fn read_input_events0, unsafe { ... } - MouseEvent::from(*input.event.MouseEvent()) is probably sound/safe?
291fn parse_key_event_record0, Several keys are dead, apparently
303fn parse_key_event_record0, Strange enumeration values for KeyEvent
345fn parse_key_event_record-1, either VK_PRIOR | VK_NEXT can be sanely simplified a lot or something is super fucky.
354fn parse_key_event_record-1, either VK_END | VK_HOME can be sanely simplified a lot or something is super fucky.
367fn parse_key_event_record0, unsafe { ... } assumes UnicodeChar is valid. Private fn only called on win32 results... technically unsound of winapi only populated AsciiChar, but that would be super dumb.
EOF

src/sys/unix.rs

LineWhatNotes
19fn get_tty_fd+1, unsafe { ... } looks sound
45fn read_char_raw0, read == 0 can probably happen when pipe broken? generates extra ' '?
EOF
positive
2019-09-28
MaulingMonkey
medium, medium

Pros:

  • Abstracts platform specific bits
  • This part is safe code

0.3.1

FileRatingNotes
.github/CODEOWNERS+1
docs/CONTRIBUTING.md+1
src/screen/alternate.rs+1
src/sceren/raw.rs+1
src/sys/unix.rs+1
src/sys/winapi.rs+1
src/lib.rs+1
src/screen.rs+1
src/sys.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travvis.yml+1No MSRV
Cargo.toml+1MIT, winapi, crossterm_*
Cargo.toml.orig+1MIT, winapi, crossterm_*
CHANGELOG.md+1
LICENSE+1MIT
README.md+1
OtherRatingNotes
unsafe+1Not in this crate
fs+1Not in this crate
io+1Indirect / stdio
docs+1
tests??Not in this crate
negative
2019-09-28
MaulingMonkey
medium, medium
issues:
high
Unguarded access of static mut ORIGINAL_CONSOLE_COLOR is unsound

Pros:

  • Styling!

Cons:

  • Poor win7 support
  • Unsound
FileRatingNotes
.github/CODEOWNERS+1
docs/CONTRIBUTING.md+1
src/enums/attribute.rs+1Verified codes vs https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_parameters
src/enums/color.rs0FromStr for Color doesn't implement RGB parsing despite supporting RGB
src/enums/colored.rs+1
src/ansi_color.rs0Verified codes vs https://en.wikipedia.org/wiki/ANSI_escape_code#8-bit . Could've simplified match logic a bit.
src/color.rs-1Bugs (#261, #263)
src/enums.rs+1
src/lib.rs+1
src/macros.rs+1
src/objectstyle.rs+1
src/styledobject.rs0Odd fg/bg naming style. Also reset seems suboptimal if nesting styles?
src/traits.rs+1Not sure how wild I am about &str extension methods, but it works.
src/winapi_color.rs-1Unsound static mut ORIGINAL_CONSOLE_COLOR if original_console_color ever called before init_console_color, which appears possible #245
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+1No MSRV
Cargo.toml+1MIT, winapi, crossterm_winapi, serde
Cargo.toml.orig+1MIT, winapi, crossterm_winapi, serde
CHANGELOG.md+1
LICENSE+1MIT
README.md+1
OtherRatingNotes
unsafe-1#245 unsound static mut
fs+1None
io0Not sure what to blame for win7 styling failures
docs+1
tests-1Few, hard anyways

src/winapi_color.rs

LineWhatNotes
46fn WinApiColor::set_fgmask should be 0x00F0 instead of special casing BACKGROUND_INTENSITY
78fn WinApiColor::set_bgmask should be 0x000F instead of special casing FOREGROUND_INTENSITY
118fn color_valueIsn't Color::White and Color::Grey here swapped in terms of colors to be used?
119fn color_valueIsn't Color::White and Color::Grey here swapped in terms of colors to be used?
133fn color_value0 seems like a poor choice for fallback fg color, especially when it's also used for bg color
153fn color_valueIsn't Color::White and Color::Grey here swapped in terms of colors to be used?
154fn color_valueIsn't Color::White and Color::Grey here swapped in terms of colors to be used?
133fn color_value0 seems like a mediocre choice for fallback bg color, especially when it's also used for fg color
172fn color_valueWait why the heck are we going to/from strings that makes 0 sense
191static mut ORIGINAL_CONSOLE_COLORUnsound access if reset called before set_??, which appears possible #245
positive
2019-09-17
MaulingMonkey
medium, low

0.4.16: Trivial version bumps.
0.4.15: Trivial version bumps.
0.4.14: Trivial version bumps, style changes.
0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11
Some of the build/path stuff seems a little off... but might be correct?
All safe code, no security problems.

Reviewed:

src\build_env.rs    +1
src\build.rs        +1
src\lib.rs          0
    102 Is this really correct for specifying the *host* environment?
    105 ..
src\utils.rs        0
    14  Isn't this generating /../../../ ?  Doesn't seem right...

Cargo.toml          +1
Cargo.toml.orig     +1
positive
2019-09-17
MaulingMonkey
medium, low

0.4.15: Trivial version bumps.
0.4.14: Trivial version bumps, style changes.
0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11
Some of the build/path stuff seems a little off... but might be correct?
All safe code, no security problems.

Reviewed:

src\build_env.rs    +1
src\build.rs        +1
src\lib.rs          0
    102 Is this really correct for specifying the *host* environment?
    105 ..
src\utils.rs        0
    14  Isn't this generating /../../../ ?  Doesn't seem right...

Cargo.toml          +1
Cargo.toml.orig     +1
positive
2019-09-17
MaulingMonkey
medium, low

0.4.14: Trivial version bumps, style changes.
0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11
Some of the build/path stuff seems a little off... but might be correct?
All safe code, no security problems.

Reviewed:

src\build_env.rs    +1
src\build.rs        +1
src\lib.rs          0
    102 Is this really correct for specifying the *host* environment?
    105 ..
src\utils.rs        0
    14  Isn't this generating /../../../ ?  Doesn't seem right...

Cargo.toml          +1
Cargo.toml.orig     +1
positive
2019-09-09
MaulingMonkey
medium, low

0.4.13: Trivial version bumps.
0.4.12: Trivial version bumps.
0.4.11
Some of the build/path stuff seems a little off... but might be correct?
All safe code, no security problems.

Reviewed:

src\build_env.rs    +1
src\build.rs        +1
src\lib.rs          0
    102 Is this really correct for specifying the *host* environment?
    105 ..
src\utils.rs        0
    14  Isn't this generating /../../../ ?  Doesn't seem right...

Cargo.toml          +1
Cargo.toml.orig     +1
positive
2019-09-03
MaulingMonkey
medium, low

0.4.12: Trivial version bumps.
0.4.11
Some of the build/path stuff seems a little off... but might be correct?
All safe code, no security problems.

Reviewed:

src\build_env.rs    +1
src\build.rs        +1
src\lib.rs          0
    102 Is this really correct for specifying the *host* environment?
    105 ..
src\utils.rs        0
    14  Isn't this generating /../../../ ?  Doesn't seem right...

Cargo.toml          +1
Cargo.toml.orig     +1
positive
2019-07-26
MaulingMonkey
medium, low

Some of the build/path stuff seems a little off... but might be correct?
All safe code, no security problems.

Reviewed:

src\build_env.rs    +1
src\build.rs        +1
src\lib.rs          0
    102 Is this really correct for specifying the *host* environment?
    105 ..
src\utils.rs        0
    14  Isn't this generating /../../../ ?  Doesn't seem right...

Cargo.toml          +1
Cargo.toml.orig     +1
neutral
2019-09-17
MaulingMonkey
low, low

0.4.16: Trivial version bumps, thread -> std::thread
0.4.15: Trivial version bumps
0.4.14: Trivial version bumps, lots of pointless style changes, a few warning fixes (missing dyns etc.)
0.4.13: Trivial version bumps
0.4.12: Trivial version bumps
0.4.11
thoroughness: low - mostly due to iOS FFI, shell stuff not being super thorough, and review fatigue causing my eyes to glaze over in places.
understanding: low - lots of shell/path stuff that I don't know well enough to verify
rating: neutral - see concerns bellow.

Concerns

  • Looks like lots of stuff might not work on windows... although there is windows-specific code, so maybe?
  • Lots of unimplemented!()
  • iOS support is chock full of unsafe { ... } for FFI. I haven't verified the FFI signatures.
  • iOS support also uses unsafe { ... } for several objective C casts. Needs some sanity checked utility functions.
  • iOS support has some potential UB during panics due to unwinding over FFI boundaries.
  • Implements some code signing stuff for iOS. Necessary, and just your local certs, but I haven't fully reasoned through what security impacts if any that has.
  • Few unit tests visible in the crate itself (maybe they're separate and unpackaged?)
  • Sandboxing concerns
    • Remotes into other devices, including over ssh.
    • Frequent use of shell commands could lead to build server RCEs given malicious project metadata
    • Malicious projects will just use a build.rs file though, front door is open so to speak.

Details

src\android\device.rs +1
14 Odd place to install...
src\android\mod.rs +1
155 Could also check android studio SDK install path

src\host\device.rs +1
src\host\mod.rs +1
src\host\platform.rs +1

src\ios\device.rs 0
50 Not sure if the underlying iOS APIs are thread safe, but this seems acceptable.
277 unsafe { ... } for FFI. Looks generally safe except for scary mem::transmute(kCFBooleanTrue), but even that may be right.
303 unsafe { ... } for FFI. Looks safe to me.
367 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
419 unsafe { ... } for FFI. Looks safe to me.
440 unsafe { ... } for FFI. Looks safe to me.
454 unsafe { ... } for FFI. Looks safe to me.
487 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
519 This thread just eats errors.
683 unsafe { ... } for FFI. Transmute... probably safe.
690 unsafe { ... } for FFI. Looks safe to me.
691 unsafe { ... } for FFI. Scary Core Foundation related transmutes... probably OK, but some utilities to sanity check these conversions in debug would be nice.
696 unsafe { ... } for FFI. Another scary-but-probably-safe transmute.
src\ios\helpers.py +1
src\ios\mobiledevice_sys.rs 0
FFI, not perfectly verified since I don't have an OS X machine to check the headers out on.
Ref: https://github.com/PanayotCankov/device.io/blob/master/idb/MobileDevice.h
Various minor const differences, a few functions missing in ref, a few likely improved definitions in places.
33 am_device_notification_callback_info has "extra" field vs reference, "subscription". iOS internal struct?
src\ios\mod.rs 0
43 unsafe { ... } for FFI. Looks safe to me.
55 technically unsound inner fn (uses ptrs)
61 Scary looking as hell, but I think this is just going Box (40) -> void* (48) -> Box (here).
62 UNDEFINED BEHAVIOR: Possible panic unwind through FFI, technically an hazard.
I would've missed this edge case but for the comment, so I'm not docking points.
Unlikely to cause severe problems, but would be worth fixing.
src\ios\platform.rs +1
src\ios\xcode.rs +1
25 Dead code not reviewed
91 I have not thoroughly audited this code signing stuff, but looks OK.
199 com.zoy.kali.Dinghy? A bit hardcoded...

src\platform\mod.rs +1
src\platform\regular_platform.rs +1
44 Since when does "regular" mean "*nix" - might not work on windows.

src\script\device.rs +1
src\script\mod.rs +1

src\ssh\device.rs 0
src\ssh\mod.rs +1

src\compiler.rs +1
475 Isn't this unbanned?
src\config.rs +1
src\device.rs +1
src\errors.rs +1
src\lib.rs +1
87 random sleep? why?
src\overlay.rs +1
src\project.rs +1
src\toolchain.rs +1
src\utils.rs +1

build.rs +1
Cargo.toml +1
Cargo.toml.orig +1

TIL

Neat loop pattern:

for (a,     b,      c) in &[
    ("a",   "b",    "c"),
    ("aa",  "bb",   "cc"),
] {
    ...
}

Sysroot paths: ndk/toolchains/llvm/prebuilt/sysroot/usr/lib/{binutils_cpu}-linux-{abi_kind}"

neutral
2019-09-17
MaulingMonkey
low, low

0.4.15: Trivial version bumps
0.4.14: Trivial version bumps, lots of pointless style changes, a few warning fixes (missing dyns etc.)
0.4.13: Trivial version bumps
0.4.12: Trivial version bumps
0.4.11
thoroughness: low - mostly due to iOS FFI, shell stuff not being super thorough, and review fatigue causing my eyes to glaze over in places.
understanding: low - lots of shell/path stuff that I don't know well enough to verify
rating: neutral - see concerns bellow.

Concerns

  • Looks like lots of stuff might not work on windows... although there is windows-specific code, so maybe?
  • Lots of unimplemented!()
  • iOS support is chock full of unsafe { ... } for FFI. I haven't verified the FFI signatures.
  • iOS support also uses unsafe { ... } for several objective C casts. Needs some sanity checked utility functions.
  • iOS support has some potential UB during panics due to unwinding over FFI boundaries.
  • Implements some code signing stuff for iOS. Necessary, and just your local certs, but I haven't fully reasoned through what security impacts if any that has.
  • Few unit tests visible in the crate itself (maybe they're separate and unpackaged?)
  • Sandboxing concerns
    • Remotes into other devices, including over ssh.
    • Frequent use of shell commands could lead to build server RCEs given malicious project metadata
    • Malicious projects will just use a build.rs file though, front door is open so to speak.

Details

src\android\device.rs +1
14 Odd place to install...
src\android\mod.rs +1
155 Could also check android studio SDK install path

src\host\device.rs +1
src\host\mod.rs +1
src\host\platform.rs +1

src\ios\device.rs 0
50 Not sure if the underlying iOS APIs are thread safe, but this seems acceptable.
277 unsafe { ... } for FFI. Looks generally safe except for scary mem::transmute(kCFBooleanTrue), but even that may be right.
303 unsafe { ... } for FFI. Looks safe to me.
367 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
419 unsafe { ... } for FFI. Looks safe to me.
440 unsafe { ... } for FFI. Looks safe to me.
454 unsafe { ... } for FFI. Looks safe to me.
487 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
519 This thread just eats errors.
683 unsafe { ... } for FFI. Transmute... probably safe.
690 unsafe { ... } for FFI. Looks safe to me.
691 unsafe { ... } for FFI. Scary Core Foundation related transmutes... probably OK, but some utilities to sanity check these conversions in debug would be nice.
696 unsafe { ... } for FFI. Another scary-but-probably-safe transmute.
src\ios\helpers.py +1
src\ios\mobiledevice_sys.rs 0
FFI, not perfectly verified since I don't have an OS X machine to check the headers out on.
Ref: https://github.com/PanayotCankov/device.io/blob/master/idb/MobileDevice.h
Various minor const differences, a few functions missing in ref, a few likely improved definitions in places.
33 am_device_notification_callback_info has "extra" field vs reference, "subscription". iOS internal struct?
src\ios\mod.rs 0
43 unsafe { ... } for FFI. Looks safe to me.
55 technically unsound inner fn (uses ptrs)
61 Scary looking as hell, but I think this is just going Box (40) -> void* (48) -> Box (here).
62 UNDEFINED BEHAVIOR: Possible panic unwind through FFI, technically an hazard.
I would've missed this edge case but for the comment, so I'm not docking points.
Unlikely to cause severe problems, but would be worth fixing.
src\ios\platform.rs +1
src\ios\xcode.rs +1
25 Dead code not reviewed
91 I have not thoroughly audited this code signing stuff, but looks OK.
199 com.zoy.kali.Dinghy? A bit hardcoded...

src\platform\mod.rs +1
src\platform\regular_platform.rs +1
44 Since when does "regular" mean "*nix" - might not work on windows.

src\script\device.rs +1
src\script\mod.rs +1

src\ssh\device.rs 0
src\ssh\mod.rs +1

src\compiler.rs +1
475 Isn't this unbanned?
src\config.rs +1
src\device.rs +1
src\errors.rs +1
src\lib.rs +1
87 random sleep? why?
src\overlay.rs +1
src\project.rs +1
src\toolchain.rs +1
src\utils.rs +1

build.rs +1
Cargo.toml +1
Cargo.toml.orig +1

TIL

Neat loop pattern:

for (a,     b,      c) in &[
    ("a",   "b",    "c"),
    ("aa",  "bb",   "cc"),
] {
    ...
}

Sysroot paths: ndk/toolchains/llvm/prebuilt/sysroot/usr/lib/{binutils_cpu}-linux-{abi_kind}"

neutral
2019-09-17
MaulingMonkey
low, low

0.4.14: Trivial version bumps, lots of pointless style changes, a few warning fixes (missing dyns etc.)
0.4.13: Trivial version bumps
0.4.12: Trivial version bumps
0.4.11
thoroughness: low - mostly due to iOS FFI, shell stuff not being super thorough, and review fatigue causing my eyes to glaze over in places.
understanding: low - lots of shell/path stuff that I don't know well enough to verify
rating: neutral - see concerns bellow.

Concerns

  • Looks like lots of stuff might not work on windows... although there is windows-specific code, so maybe?
  • Lots of unimplemented!()
  • iOS support is chock full of unsafe { ... } for FFI. I haven't verified the FFI signatures.
  • iOS support also uses unsafe { ... } for several objective C casts. Needs some sanity checked utility functions.
  • iOS support has some potential UB during panics due to unwinding over FFI boundaries.
  • Implements some code signing stuff for iOS. Necessary, and just your local certs, but I haven't fully reasoned through what security impacts if any that has.
  • Few unit tests visible in the crate itself (maybe they're separate and unpackaged?)
  • Sandboxing concerns
    • Remotes into other devices, including over ssh.
    • Frequent use of shell commands could lead to build server RCEs given malicious project metadata
    • Malicious projects will just use a build.rs file though, front door is open so to speak.

Details

src\android\device.rs +1
14 Odd place to install...
src\android\mod.rs +1
155 Could also check android studio SDK install path

src\host\device.rs +1
src\host\mod.rs +1
src\host\platform.rs +1

src\ios\device.rs 0
50 Not sure if the underlying iOS APIs are thread safe, but this seems acceptable.
277 unsafe { ... } for FFI. Looks generally safe except for scary mem::transmute(kCFBooleanTrue), but even that may be right.
303 unsafe { ... } for FFI. Looks safe to me.
367 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
419 unsafe { ... } for FFI. Looks safe to me.
440 unsafe { ... } for FFI. Looks safe to me.
454 unsafe { ... } for FFI. Looks safe to me.
487 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
519 This thread just eats errors.
683 unsafe { ... } for FFI. Transmute... probably safe.
690 unsafe { ... } for FFI. Looks safe to me.
691 unsafe { ... } for FFI. Scary Core Foundation related transmutes... probably OK, but some utilities to sanity check these conversions in debug would be nice.
696 unsafe { ... } for FFI. Another scary-but-probably-safe transmute.
src\ios\helpers.py +1
src\ios\mobiledevice_sys.rs 0
FFI, not perfectly verified since I don't have an OS X machine to check the headers out on.
Ref: https://github.com/PanayotCankov/device.io/blob/master/idb/MobileDevice.h
Various minor const differences, a few functions missing in ref, a few likely improved definitions in places.
33 am_device_notification_callback_info has "extra" field vs reference, "subscription". iOS internal struct?
src\ios\mod.rs 0
43 unsafe { ... } for FFI. Looks safe to me.
55 technically unsound inner fn (uses ptrs)
61 Scary looking as hell, but I think this is just going Box (40) -> void* (48) -> Box (here).
62 UNDEFINED BEHAVIOR: Possible panic unwind through FFI, technically an hazard.
I would've missed this edge case but for the comment, so I'm not docking points.
Unlikely to cause severe problems, but would be worth fixing.
src\ios\platform.rs +1
src\ios\xcode.rs +1
25 Dead code not reviewed
91 I have not thoroughly audited this code signing stuff, but looks OK.
199 com.zoy.kali.Dinghy? A bit hardcoded...

src\platform\mod.rs +1
src\platform\regular_platform.rs +1
44 Since when does "regular" mean "*nix" - might not work on windows.

src\script\device.rs +1
src\script\mod.rs +1

src\ssh\device.rs 0
src\ssh\mod.rs +1

src\compiler.rs +1
475 Isn't this unbanned?
src\config.rs +1
src\device.rs +1
src\errors.rs +1
src\lib.rs +1
87 random sleep? why?
src\overlay.rs +1
src\project.rs +1
src\toolchain.rs +1
src\utils.rs +1

build.rs +1
Cargo.toml +1
Cargo.toml.orig +1

TIL

Neat loop pattern:

for (a,     b,      c) in &[
    ("a",   "b",    "c"),
    ("aa",  "bb",   "cc"),
] {
    ...
}

Sysroot paths: ndk/toolchains/llvm/prebuilt/sysroot/usr/lib/{binutils_cpu}-linux-{abi_kind}"

neutral
2019-09-09
MaulingMonkey
low, low

0.4.13: Trivial version bumps
0.4.12: Trivial version bumps
0.4.11
thoroughness: low - mostly due to iOS FFI, shell stuff not being super thorough, and review fatigue causing my eyes to glaze over in places.
understanding: low - lots of shell/path stuff that I don't know well enough to verify
rating: neutral - see concerns bellow.

Concerns

  • Looks like lots of stuff might not work on windows... although there is windows-specific code, so maybe?
  • Lots of unimplemented!()
  • iOS support is chock full of unsafe { ... } for FFI. I haven't verified the FFI signatures.
  • iOS support also uses unsafe { ... } for several objective C casts. Needs some sanity checked utility functions.
  • iOS support has some potential UB during panics due to unwinding over FFI boundaries.
  • Implements some code signing stuff for iOS. Necessary, and just your local certs, but I haven't fully reasoned through what security impacts if any that has.
  • Few unit tests visible in the crate itself (maybe they're separate and unpackaged?)
  • Sandboxing concerns
    • Remotes into other devices, including over ssh.
    • Frequent use of shell commands could lead to build server RCEs given malicious project metadata
    • Malicious projects will just use a build.rs file though, front door is open so to speak.

Details

src\android\device.rs +1
14 Odd place to install...
src\android\mod.rs +1
155 Could also check android studio SDK install path

src\host\device.rs +1
src\host\mod.rs +1
src\host\platform.rs +1

src\ios\device.rs 0
50 Not sure if the underlying iOS APIs are thread safe, but this seems acceptable.
277 unsafe { ... } for FFI. Looks generally safe except for scary mem::transmute(kCFBooleanTrue), but even that may be right.
303 unsafe { ... } for FFI. Looks safe to me.
367 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
419 unsafe { ... } for FFI. Looks safe to me.
440 unsafe { ... } for FFI. Looks safe to me.
454 unsafe { ... } for FFI. Looks safe to me.
487 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
519 This thread just eats errors.
683 unsafe { ... } for FFI. Transmute... probably safe.
690 unsafe { ... } for FFI. Looks safe to me.
691 unsafe { ... } for FFI. Scary Core Foundation related transmutes... probably OK, but some utilities to sanity check these conversions in debug would be nice.
696 unsafe { ... } for FFI. Another scary-but-probably-safe transmute.
src\ios\helpers.py +1
src\ios\mobiledevice_sys.rs 0
FFI, not perfectly verified since I don't have an OS X machine to check the headers out on.
Ref: https://github.com/PanayotCankov/device.io/blob/master/idb/MobileDevice.h
Various minor const differences, a few functions missing in ref, a few likely improved definitions in places.
33 am_device_notification_callback_info has "extra" field vs reference, "subscription". iOS internal struct?
src\ios\mod.rs 0
43 unsafe { ... } for FFI. Looks safe to me.
55 technically unsound inner fn (uses ptrs)
61 Scary looking as hell, but I think this is just going Box (40) -> void* (48) -> Box (here).
62 UNDEFINED BEHAVIOR: Possible panic unwind through FFI, technically an hazard.
I would've missed this edge case but for the comment, so I'm not docking points.
Unlikely to cause severe problems, but would be worth fixing.
src\ios\platform.rs +1
src\ios\xcode.rs +1
25 Dead code not reviewed
91 I have not thoroughly audited this code signing stuff, but looks OK.
199 com.zoy.kali.Dinghy? A bit hardcoded...

src\platform\mod.rs +1
src\platform\regular_platform.rs +1
44 Since when does "regular" mean "*nix" - might not work on windows.

src\script\device.rs +1
src\script\mod.rs +1

src\ssh\device.rs 0
src\ssh\mod.rs +1

src\compiler.rs +1
475 Isn't this unbanned?
src\config.rs +1
src\device.rs +1
src\errors.rs +1
src\lib.rs +1
87 random sleep? why?
src\overlay.rs +1
src\project.rs +1
src\toolchain.rs +1
src\utils.rs +1

build.rs +1
Cargo.toml +1
Cargo.toml.orig +1

TIL

Neat loop pattern:

for (a,     b,      c) in &[
    ("a",   "b",    "c"),
    ("aa",  "bb",   "cc"),
] {
    ...
}

Sysroot paths: ndk/toolchains/llvm/prebuilt/sysroot/usr/lib/{binutils_cpu}-linux-{abi_kind}"

neutral
2019-09-03
MaulingMonkey
low, low

0.4.12: Trivial version bumps
0.4.11
thoroughness: low - mostly due to iOS FFI, shell stuff not being super thorough, and review fatigue causing my eyes to glaze over in places.
understanding: low - lots of shell/path stuff that I don't know well enough to verify
rating: neutral - see concerns bellow.

Concerns

  • Looks like lots of stuff might not work on windows... although there is windows-specific code, so maybe?
  • Lots of unimplemented!()
  • iOS support is chock full of unsafe { ... } for FFI. I haven't verified the FFI signatures.
  • iOS support also uses unsafe { ... } for several objective C casts. Needs some sanity checked utility functions.
  • iOS support has some potential UB during panics due to unwinding over FFI boundaries.
  • Implements some code signing stuff for iOS. Necessary, and just your local certs, but I haven't fully reasoned through what security impacts if any that has.
  • Few unit tests visible in the crate itself (maybe they're separate and unpackaged?)
  • Sandboxing concerns
    • Remotes into other devices, including over ssh.
    • Frequent use of shell commands could lead to build server RCEs given malicious project metadata
    • Malicious projects will just use a build.rs file though, front door is open so to speak.

Details

src\android\device.rs +1
14 Odd place to install...
src\android\mod.rs +1
155 Could also check android studio SDK install path

src\host\device.rs +1
src\host\mod.rs +1
src\host\platform.rs +1

src\ios\device.rs 0
50 Not sure if the underlying iOS APIs are thread safe, but this seems acceptable.
277 unsafe { ... } for FFI. Looks generally safe except for scary mem::transmute(kCFBooleanTrue), but even that may be right.
303 unsafe { ... } for FFI. Looks safe to me.
367 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
419 unsafe { ... } for FFI. Looks safe to me.
440 unsafe { ... } for FFI. Looks safe to me.
454 unsafe { ... } for FFI. Looks safe to me.
487 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
519 This thread just eats errors.
683 unsafe { ... } for FFI. Transmute... probably safe.
690 unsafe { ... } for FFI. Looks safe to me.
691 unsafe { ... } for FFI. Scary Core Foundation related transmutes... probably OK, but some utilities to sanity check these conversions in debug would be nice.
696 unsafe { ... } for FFI. Another scary-but-probably-safe transmute.
src\ios\helpers.py +1
src\ios\mobiledevice_sys.rs 0
FFI, not perfectly verified since I don't have an OS X machine to check the headers out on.
Ref: https://github.com/PanayotCankov/device.io/blob/master/idb/MobileDevice.h
Various minor const differences, a few functions missing in ref, a few likely improved definitions in places.
33 am_device_notification_callback_info has "extra" field vs reference, "subscription". iOS internal struct?
src\ios\mod.rs 0
43 unsafe { ... } for FFI. Looks safe to me.
55 technically unsound inner fn (uses ptrs)
61 Scary looking as hell, but I think this is just going Box (40) -> void* (48) -> Box (here).
62 UNDEFINED BEHAVIOR: Possible panic unwind through FFI, technically an hazard.
I would've missed this edge case but for the comment, so I'm not docking points.
Unlikely to cause severe problems, but would be worth fixing.
src\ios\platform.rs +1
src\ios\xcode.rs +1
25 Dead code not reviewed
91 I have not thoroughly audited this code signing stuff, but looks OK.
199 com.zoy.kali.Dinghy? A bit hardcoded...

src\platform\mod.rs +1
src\platform\regular_platform.rs +1
44 Since when does "regular" mean "*nix" - might not work on windows.

src\script\device.rs +1
src\script\mod.rs +1

src\ssh\device.rs 0
src\ssh\mod.rs +1

src\compiler.rs +1
475 Isn't this unbanned?
src\config.rs +1
src\device.rs +1
src\errors.rs +1
src\lib.rs +1
87 random sleep? why?
src\overlay.rs +1
src\project.rs +1
src\toolchain.rs +1
src\utils.rs +1

build.rs +1
Cargo.toml +1
Cargo.toml.orig +1

TIL

Neat loop pattern:

for (a,     b,      c) in &[
    ("a",   "b",    "c"),
    ("aa",  "bb",   "cc"),
] {
    ...
}

Sysroot paths: ndk/toolchains/llvm/prebuilt/sysroot/usr/lib/{binutils_cpu}-linux-{abi_kind}"

neutral
2019-07-27
MaulingMonkey
low, low

thoroughness: low - mostly due to iOS FFI, shell stuff not being super thorough, and review fatigue causing my eyes to glaze over in places.
understanding: low - lots of shell/path stuff that I don't know well enough to verify
rating: neutral - see concerns bellow.

Concerns

  • Looks like lots of stuff might not work on windows... although there is windows-specific code, so maybe?
  • Lots of unimplemented!()
  • iOS support is chock full of unsafe { ... } for FFI. I haven't verified the FFI signatures.
  • iOS support also uses unsafe { ... } for several objective C casts. Needs some sanity checked utility functions.
  • iOS support has some potential UB during panics due to unwinding over FFI boundaries.
  • Implements some code signing stuff for iOS. Necessary, and just your local certs, but I haven't fully reasoned through what security impacts if any that has.
  • Few unit tests visible in the crate itself (maybe they're separate and unpackaged?)
  • Sandboxing concerns
    • Remotes into other devices, including over ssh.
    • Frequent use of shell commands could lead to build server RCEs given malicious project metadata
    • Malicious projects will just use a build.rs file though, front door is open so to speak.

Details

src\android\device.rs +1
14 Odd place to install...
src\android\mod.rs +1
155 Could also check android studio SDK install path

src\host\device.rs +1
src\host\mod.rs +1
src\host\platform.rs +1

src\ios\device.rs 0
50 Not sure if the underlying iOS APIs are thread safe, but this seems acceptable.
277 unsafe { ... } for FFI. Looks generally safe except for scary mem::transmute(kCFBooleanTrue), but even that may be right.
303 unsafe { ... } for FFI. Looks safe to me.
367 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
419 unsafe { ... } for FFI. Looks safe to me.
440 unsafe { ... } for FFI. Looks safe to me.
454 unsafe { ... } for FFI. Looks safe to me.
487 unsafe { ... } for FFI. Looks safe to me, but needlessly large.
519 This thread just eats errors.
683 unsafe { ... } for FFI. Transmute... probably safe.
690 unsafe { ... } for FFI. Looks safe to me.
691 unsafe { ... } for FFI. Scary Core Foundation related transmutes... probably OK, but some utilities to sanity check these conversions in debug would be nice.
696 unsafe { ... } for FFI. Another scary-but-probably-safe transmute.
src\ios\helpers.py +1
src\ios\mobiledevice_sys.rs 0
FFI, not perfectly verified since I don't have an OS X machine to check the headers out on.
Ref: https://github.com/PanayotCankov/device.io/blob/master/idb/MobileDevice.h
Various minor const differences, a few functions missing in ref, a few likely improved definitions in places.
33 am_device_notification_callback_info has "extra" field vs reference, "subscription". iOS internal struct?
src\ios\mod.rs 0
43 unsafe { ... } for FFI. Looks safe to me.
55 technically unsound inner fn (uses ptrs)
61 Scary looking as hell, but I think this is just going Box (40) -> void* (48) -> Box (here).
62 UNDEFINED BEHAVIOR: Possible panic unwind through FFI, technically an hazard.
I would've missed this edge case but for the comment, so I'm not docking points.
Unlikely to cause severe problems, but would be worth fixing.
src\ios\platform.rs +1
src\ios\xcode.rs +1
25 Dead code not reviewed
91 I have not thoroughly audited this code signing stuff, but looks OK.
199 com.zoy.kali.Dinghy? A bit hardcoded...

src\platform\mod.rs +1
src\platform\regular_platform.rs +1
44 Since when does "regular" mean "*nix" - might not work on windows.

src\script\device.rs +1
src\script\mod.rs +1

src\ssh\device.rs 0
src\ssh\mod.rs +1

src\compiler.rs +1
475 Isn't this unbanned?
src\config.rs +1
src\device.rs +1
src\errors.rs +1
src\lib.rs +1
87 random sleep? why?
src\overlay.rs +1
src\project.rs +1
src\toolchain.rs +1
src\utils.rs +1

build.rs +1
Cargo.toml +1
Cargo.toml.orig +1

TIL

Neat loop pattern:

for (a,     b,      c) in &[
    ("a",   "b",    "c"),
    ("aa",  "bb",   "cc"),
] {
    ...
}

Sysroot paths: ndk/toolchains/llvm/prebuilt/sysroot/usr/lib/{binutils_cpu}-linux-{abi_kind}"

negative
2020-07-23
MaulingMonkey
none, medium
alternative:
glutin_egl_sys
issues:
high
Unsound as heck, unmaintained.

AVOID. This crate is unsound as fuck, and abandoned.

Alternatives:

  • khronos-egl<!-- raw HTML omitted --> is a sounder, maintained fork.
  • egli<!-- raw HTML omitted --> is an alternative, supposedly sound API with low and high level APIs.
  • glutin_egl_sys<!-- raw HTML omitted --> uses gl_generator to provide low level unsafe API structs.

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/egl.md

neutral
2019-12-27
MaulingMonkey
none, medium
advisories:
high

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/foreign-types.md#050

negative
2019-12-27
MaulingMonkey
low, medium
issues:
medium
Hides unsound C APIs without user-facing unsafe keyword

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/foreign-types.md#040--032

negative
2019-07-27
MaulingMonkey
medium, medium

foreign_type! can generate unsound code without using the unsafe keyword in client code at all.

  • Invokes C FFI which may be unsafe from safe fns.
  • More recent versions (0.4.0) can implement unsafe traits (Send, Sync) without the unsafe keyword at all.
negative
2019-07-27
MaulingMonkey
medium, low

Technically sound, but makes me nervous.

Opaque: Looks way too dereferencable, and would have the wrong size - that of ().
ForeignTypeRef: Unchecked pointer casts galore.

negative
2019-09-27
MaulingMonkey
none, none
issues:
high
Unsound API due to debug-only bounds checks

Yet another unsound ECS. Saw more 16-bit counters too.

positive
2019-07-23
MaulingMonkey
medium, medium

I'm assuming GetVolumePathNameW will leave buffer null terminated if successful. Unsafe only used for sane-looking FFI, and zeroing structs.

positive
2019-07-28
MaulingMonkey
low, medium

Looks nice and solid. Well documented, well tested, code coverage, safe + sound.
Large crate - so I haven't taken the time to thoroughly audit everything - but I have at least skimmed over all code.
I've written some of my own partial DWARF parser in C++ in the past... parts look appropriately familiar.

Detail

FileRatingNotes
benches/bench.rs+1fs
examples/dwarfdump.rs+1fs
fixtures/self/*unreviewed binary data (used in test fixtures, probably OK)
fixtures/self/README.md+1
releases/friends.sh+1
releases/release-announcement-template.md+1
src/abbrev.rs+1
src/aranges.rs+1
src/cfi.rs+1
src/constants.rs+1
src/endianity.rs+1
src/leb128.rs+1
src/lib.rs+1
src/line.rs+1
src/loc.rs+1
src/lookup.rs+1
src/op.rs+1
src/parser.rs+1
src/pubnames.rs+1
src/pubtypes.rs+1
src/ranges.rs+1
src/reader.rs+1
src/str.rs+1
src/test_util.rs+1
src/unit.rs+1
tests/parse_self.rs+1
.cargo-ok+1
.gitignore+1
.travis.yml0I haven't reviewed travis-cargo
Cargo.toml+1
Cargo.toml.orig+1
CONTRIBUTING.md+1
coverage+1
format+1
LICENSE-APACHE+1
LICENSE-MIT+1
README.md+1
rustfmt.toml+1
OtherRatingNotes
unsafe+1Single match unsafe { memmap::Mmap::map(&file) }
fs+1Safe, only used in benches/examples/tests
io+1Some io::{stderr, Error, Write} - all sane
docs+1Lots of 'em!
tests+1Lots of 'em!

TIL

travis-cargo

positive
2019-09-24
MaulingMonkey
medium, high

Handle basic #include s for GLSL.

Pros:

  • 100% Safe/Sandboxed

Cons:

  • No #file directives emitted
  • No callback option for #include, must pre-register all possible includes.
  • Repeat #include s simply ignored instead of interacting with preprocessor.

0.3.1

crev
thoroughnessmedium
understandinghigh
ratingpositive
FileRatingNotes
src/error.rs+1Display for Error not double-clickable but provides good context.
src/lib.rs0Doesn't disambiguate quote style, no callbacks so you must pre-define every includable file.
.cargo_vs_info.json+1
.cargo-ok+1
Cargo.toml+1Apache 2.0 or MIT
Cargo.toml.orig+1regex, lazy_static, [dev] indoc, criterion
LICENSE-APACHE+1
LICENSE-MIT+1
README.md+1Apache 2.0 or MIT, Contributions section
OtherRatingNotes
unsafe+1None, 100% safe
miri
fs+1None, instead you need to pre-include(path, into_string).
io+1
docs+1
tests+1Not in .crate, but appears to be there/fine in original repository
positive
2020-08-28
MaulingMonkey
medium, high

Encoders/decoders for .ico and .cur file formats.

Pros:

  • Well documented and ~complete despite 0.1.0 version
  • Pure Rust

Full Audit

positive
2020-08-29
MaulingMonkey
low, medium

Pros:

  • Rescaling via image
  • SVG rendering via resvg
  • Pretty nifty and relatively comprehensive
  • icon-pie's lack of multithreading makes icon-pie sound despite icon_baker's bogus Send+Sync impls

Full Audit

negative
2020-08-28
MaulingMonkey
medium, medium
issues:
high
`unsafe impl Send/Sync for Image {}` is unsound (can clone Rc from multiple threads)

Pros:

  • Rescaling via image
  • SVG rendering via resvg
  • Pretty nifty and relatively comprehensive

Cons:

  • Unsound

Full Audit

positive
2019-09-03
MaulingMonkey
low, medium

0.2.0: Some minor refactoring and a lot of ugly rustfmt(?) reformatting.
0.1.5: Read all code, including skimmed every line of autogenerated rust tables. Looked for any security issues, including any obvious DoS attacks from loops. Did not try to manually verify any of the bidi/punycode/idna logic against a spec, but test coverage looks excellent.

positive
2019-07-23
MaulingMonkey
low, medium

Read all code, including skimmed every line of autogenerated rust tables. Looked for any security issues, including any obvious DoS attacks from loops. Did not try to manually verify any of the bidi/punycode/idna logic against a spec, but test coverage looks excellent.

positive
2019-10-13
MaulingMonkey
medium, high

std::time::Instant alternative that doesn't panic on wasm targets.

Pros:

  • Doesn't panic!
  • Just std::time::Instant on native, no performance hit or anything

Cons:

  • Just std::time::Instant on native, easy to accidentally use new std APIs unavailable on wasm
  • f64 repr for Instant makes me nervous
  • Unusual license choice for rust projects
  • Could use more test coverage
FileRatingNotes
.circleci/config.yml+1
src/lib.rs+1
src/native.rs+1I would've preferred a wrap Instant for ensuring compat, but sure.
src/wasm.rs0f64 repr might accumulate poorly
tests/wasm.rs0test_instant_now could spuriously fail if elapsed() == 0, would like to see more test coverage
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1Overkill
AUTHORS+1
Cargo.lock0Pointless, shouldn't be part of the package
Cargo.toml+1BSD-3-Clause
Cargo.toml.orig+1BSD-3-Clause
LICENSE+1BSD-3-Clause?
README.md+1
OtherRatingNotes
unsafe+1None
fs+1None
io+1None
docs0Mostly unnecessary
tests0Could use more
positive
2019-12-27
MaulingMonkey
high, high

Great crate. Doesn't work with WASM.
https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/inventory.md#015

positive
2019-12-27
MaulingMonkey
high, high
advisories:
high

Great crate. Doesn't work with WASM.
https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/inventory.md#015

negative
2019-12-27
MaulingMonkey
high, high
issues:
high
Soundness issues with pre-main multithreading

Great crate, but currently unsound. Also doesn't work with WASM.
https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/inventory.md#014

neutral
2019-07-26
MaulingMonkey
low, low

I believe this is sound, but has way too much unsafe and too few tests for my tastes.
udivmod_1e19 reduces my thoroughness and understanding to low, otherwise both would be medium or higher.

Reviewed:
benches/bench.rs +1

src/lib.rs          0
    119 mem::uninitialized (POD)
    164 impl_IntegerCommon is potentially unsound, but not public, and guarded by debug_assert!s at least.
        Would vastly prefer static_assertions though.
    196 one giant unsafe block for pointer math?  No bounds checks?  Super gross!
        Relies on $max_len being enough for $t with no bounds checking.
        Relies on d1/d2/n being unable to overflow DEC_DIGITS_LUT.
        A careful reading makes this appear technically sound, but super super sketchy.
    246 Carefully audited for impl_Integer.
    255 Carefully audited for impl_Integer.
    267 Carefully audited for impl_Integer.
    270 Carefully audited for impl_Integer.
    273 Carefully audited for impl_Integer.
    294 Again?  A careful reading makes this appear technically sound, but super super sketchy.
    338 Carefully audited for impl_Integer128.
    339 Shouldn't this technically have the cfg if U128_MAX_LEN is going to have it?
    341 Carefully audited for impl_Integer128.

tests/test.rs       +1
    Not nearly enough tests for my liking given how much unsafe code there is.
    I'd like to see every edge case of iota::write tested, they aren't.
    I'd like to see every edge case of udivmod_1e19 tested, it's not directly tested at all.

.gitignore          +1
.travis.yml         +1
Cargo.toml          +1
Cargo.toml.orig     +1
README.md           +1

Not yet fully reviewed:

src/udiv128.rs      0
    34  Correct - since high is nonzero (line 29 bails if it wasn't), leading_zeros < 64.
    35+ My eyes glazed over at this point.  I haven't verified udivmod_1e19(...).1 meets it's invariants!

TIL:
You can use \``edition2018on doc comment code blocks. You can have a public trait require a private one to "seal" it. You can specifysudo: false` in .travis.yml

negative
2019-07-31
MaulingMonkey
low, medium
issues:
high
Definite soundness issue

Rated files were at least reviewed to thoroughness + understanding medium, but the rest was only reviewed to througuhness low.

Detail

FileRatingNotes
.github/PULL_REQUEST_TEMPLATE.md+1
.travis/run_travis_job.sh+1
.vscode/tasks.json+1
benches/api_calls.rs+1Various API concerns but this file is fine.
examples/HelloWorld.h+1Matches HelloWorld.java... generated by javah?
examples/HelloWorld.java+1
examples/Makefile+1
src/wrapper/descriptors/class_desc.rs+1
src/wrapper/descriptors/desc.rs+1
src/wrapper/descriptors/exception_desc.rs+1
src/wrapper/descriptors/field_desc.rs+1
src/wrapper/descriptors/method_desc.rs+1
src/wrapper/descriptors/mod.rs+1
src/wrapper/java_vm/init_args.rs+1
src/wrapper/java_vm/mod.rs+1
src/wrapper/java_vm/vm.rs-1Not 100% sure if it's sound to detatch threads out from under other Java code. Some unsafe attach fns also confuse me as to why they're unsafe.
src/wrapper/objects/auto_local.rs-1Not 100% sure if AutoLocal::new is sound based on scary JVM crash warnings
src/wrapper/objects/global_ref.rs+1
src/wrapper/objects/jbytebuffer.rs-1Allowing construction from arbitrary jobject s will likely be unsound later
src/wrapper/objects/jclass.rs-1Ditto
src/wrapper/objects/jfieldid.rs-1Ditto
src/wrapper/objects/jlist.rs-1Called it - internal is used in safe fns, unsound looking as fuck.
src/wrapper/objects/jmap.rs
src/wrapper/objects/jmethodid.rs
src/wrapper/objects/jobject.rs
src/wrapper/objects/jstaticfieldid.rs
src/wrapper/objects/jstaticmethodid.rs
src/wrapper/objects/jstring.rs
src/wrapper/objects/jthrowable.rs
src/wrapper/objects/jvalue.rs
src/wrapper/objects/mod.rs
src/wrapper/strings/ffi_str.rs
src/wrapper/strings/java_str.rs
src/wrapper/strings/mod.rs
src/wrapper/errors.rs
src/wrapper/executor.rs
src/wrapper/jnienv.rs
src/wrapper/macros.rs
src/wrapper/signature.rs
src/wrapper/version.rs
src/lib.rs
src/sys.rs+1pub use jni_sys::*
tests/util/example_proxy.rs
tests/util/mod.rs
tests/executor_nested_attach.rs
tests/executor.rs
tests/java_integers.rs
tests/jmap.rs
tests/jni_api.rs
tests/jni_global_ref_is_deleted.rs
tests/jni_global_refs.rs
tests/threads_attach_guard.rs
tests/threads_detach_daemon.rs
tests/threads_detach.rs
tests/threads_explicit_detach_daemon.rs
tests/threads_explicit_detach_permanent.rs
tests/threads_explicit_detach.rs
tests/threads_nested_attach_daemon.rs
tests/threads_nested_attach_guard.rs
tests/threads_nested_attach_permanently.rs
.appveyor.yml
.gitignore
.travis.yml
Cargo.toml+1
Cargo.toml.orig+1
CHANGELOG.md+1
clippy.toml+1
CODE_OF_CONDUCT.md+1
CONTRIBUTING.md+1
LICENSE-APACHE+1
LICENSE-MIT+1
README.md+1
OtherRatingNotes
unsafe-1Unsound
fs+1Not used
io+1Not used
docs+1Well documented
tests+1Decent testing

benches/api_calls.rs

LineNotes
52..._unchecked is safe? Look at call_static_method_unchecked carefully.
69Not all ..._unchecked are safe?
154Must manually drop local refs? Lame.
226No use of black box?
+1

src/wrapper/descriptors/method_desc.rs

LineNotes
24I feel like having an implicit "<init>" instead of a struct of some sort is potentially confusing?
+1

src/wrapper/java_vm/init_args.rs

LineNotes
46Could use more doc-tests
50Silently ignoring unsupported options is a little lame
70JavaVM::build in doc comments, not new ?
101Pretty gosh darn heckin' sketchy if you ask me... relies on opts never being modified after this point. Fortunately this type's contents are nice and private/local, so that's enforced.
+1

src/wrapper/java_vm/vm.rs

LineNotes
134unsafe impl Send + Sync - I believe this is safe for JavaVM (as used here), but not for JNIEnv (keep a look out for that later)
150unsafe { ... } - ptr casts are a bit sketchy, otherwise LGTM.
158+1
165unsafe fn - +1
185attach_current_thread_permanently - possible noop if already attached, meaning it might be temporary!
232detach_current_thread - doc comments make this sound possibly unsound?
270unsafe { ... } - +1
280unsafe { ... } - +1
364unsafe fn - This... actually looks sound? What am I missing?
386unsafe fn - This... actually looks sound? What am I missing?
409unsafe { ... } - +1
-1 - Can detatched threads cause unsoundness? What am I missing for unsafe fn?

src/wrapper/objects/global_ref.rs

LineNotes
36unsafe impl Send + Sync - should be safe?
48unsafe fn - presumably because this takes a jobject. LGTM.
66unsafe fn - presumably because this takes a jobject. LGTM.
+1

src/wrapper/objects/jbytebuffer.rs

LineNotes
11jobject is just a pointer, so this general purpouse crate-exported method means using JByteBuffer s is unsafe!
32there's no guarantee a given JObject is a JByteBuffer, but this succeeds unconditionally.
-1 - I suspect invalid jobject s will cause soundness issues later

src/wrapper/objects/jlist.rs

LineNotes
20Eww, methods cached per-object?
46jobject -> JObject -> JList can be constructed with an invalid pointer...
69...making all safe fns on this type unsound. Use of 'safe' _unchecked methods also concerns me.
-1

src/wrapper/macros.rs

LineRatingNotes
...
26+1non_null
...
105+1java_vm_unchecked - 'unchecked' refers to error codes. unsafe macro, $java_vm must be valid.
132-1java_vm_method - I wish this forced the caller to use unsafe { ... }. unsafe macro, $jnienv must be valid.
...

TIL

  • Use javah to generate rust, perhaps?
  • [build-dependencies]
positive
2019-07-30
MaulingMonkey
medium, high

Good solid FFI crate. Manual generation (I see comments!) concerns me, but upon review it looks to have been done correctly.

Verified all structs and FFI signatures against android JNI.h, with the exception of double checking that everything is marked JNICALL on windows JNI instead of just most of it.
Android JNI lacks parameter names, so I didn't sanity check those either.
Verified against %LOCALAPPDATA%\Android\Sdk\ndk-bundle\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\include\jni.h

Detail

FileRatingNotes
src/lib.rs+1Lots of unsafe... but necessary.
.gitignore+1
.travis.yml+1
appveyor.yml+1
Cargo.toml+1
Cargo.toml.orig+1
README.md+1
OtherRatingNotes
unsafe+1Lots of it... but all necessary, and after careful review, all appears correct.
fs+1None
io+1None
docs-1Nonexistant, but of low importance (other primary references are available)
tests0Not in crate

src/lib.rs

LineRatingNotes
7+1Looks sufficiently correct to me - VS defines this as *mut c_char, but *mut c_void is close enough IMO.
10+1all verified
200native version uses more newtypes, but without inheritence maybe this is sane in Rust.
39+1verified
51+1sure
57+1
64-1DANGER - rust enum for C enum is begging for unsound if returned from FFI
71+1JNI constants here all look good
930JNINativeMethod - C version uses const char*, but the muts here aren't a big deal... I
99+1
105+1JNINativeInterface is bloody huge, but all methods appear to match in name, position, and signature...
115...with the caveat that there's a lot of "system" convention when JNICALL isn't used throughout the android jni.h. However, it is used in openjdk so that's probably fine?
116I'm also assuming Option<unsafe extern "system" fn(...) -> ...> is FFI compatible with C function pointers, which might be a bad assumption.
157Native version isn't marked no return but I bet it is.
175Also, varargs functions degrade to "C" mode per https://stackoverflow.com/a/3615407
1411Reviewed all the way to here!
1413+1Sure
1421+1JNIEnv_ - Lack of impl with forwarding fns like jni.h has is vageuly annoying.
1425+1Sure
1433+1JavaVMOption - Again, mut differences, but otherwise OK. Sure.
1438+1Sure
1446+1JavaVMInitArgs - LGTM
1453+1Sure
1461+1JavaVMAttachArgs - Again, mut differences, but otherwise OK. Sure.
1467+1Sure
1475+1JNIInvokeInterface - LGTM
1501+1Sure
1507+1LHTM
positive
2019-09-03
MaulingMonkey
high, high

1.4.0: Read diff, looks fine.
1.3.0: Read all of src, skimmed all of tests. core_lazy.rs looks a little odd, but is 100% safe code - any issues would be in it's core dependency, spin. inline_lazy.rs contains unsafe blocks... look safe, but downgrades rating to merely positive. lib.rs is just safe macros. Tests all pass.

positive
2019-07-23
MaulingMonkey
high, high

Read all of src, skimmed all of tests. core_lazy.rs looks a little odd, but is 100% safe code - any issues would be in it's core dependency, spin. inline_lazy.rs contains unsafe blocks... look safe, but downgrades rating to merely positive. lib.rs is just safe macros. Tests all pass.

negative
2019-07-24
MaulingMonkey
medium, medium

lazy_static 0.2.11 relied on unsafe muts which would have race conditions during initialization.
I recommend upgrading to a modern lazy_static 1.3.0+!

src\core_lazy.rs: +1
src\lazy.rs: -1 UNSOUND (race conditions in multithreaded init due to static mut / mut refs taken outside of call_once)
src\lib.rs: -1 UNSOUND (uses the static muts without syncronization)
src\nightly_lazy.rs: -1 UNSOUND (race conditions in multithreaded init due to static mut / mut refs taken outside of call_once)

tests*: +1 (skimmed, seem fine)

.travis.yml: +1
.appveyor.yml: +1
Cargo.toml: +1
Cargo.toml.orig: +1
README.md: +1

positive
2020-01-29
MaulingMonkey
high, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/lazycell.md#1.2.1
Appears quite high quality, only the minimum necessary amount of unsafe code used.

positive
2020-01-29
MaulingMonkey
high, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/lazycell.md#1.2.0
Appears quite high quality, only the minimum necessary amount of unsafe code used.

positive
2021-03-28
MaulingMonkey
medium, high

Encode/decode DWARF's variable length integer format, LEB128

Pros:

  • Apache-2.0 OR MIT
  • Good test coverage
  • Lean

Full Audit

positive
2021-03-28
MaulingMonkey
medium, high

Encode/decode DWARF's variable length integer format, LEB128

Pros:

  • Apache-2.0 OR MIT
  • Good test coverage
  • Lean

Full Audit

positive
2021-03-28
MaulingMonkey
medium, high

Encode/decode DWARF's variable length integer format, LEB128

Pros:

  • Apache-2.0 OR MIT
  • Good test coverage
  • Lean

Full Audit

positive
2021-03-28
MaulingMonkey
medium, high

Encode/decode DWARF's variable length integer format, LEB128

Pros:

  • Apache-2.0 OR MIT
  • Good test coverage
  • Lean

Full Audit

positive
2021-03-28
MaulingMonkey
medium, high

Encode/decode DWARF's variable length integer format, LEB128

Pros:

  • Apache-2.0 OR MIT
  • Good test coverage
  • Lean

Full Audit

negative
2021-03-28
MaulingMonkey
medium, high

Encode/decode DWARF's variable length integer format, LEB128

Pros:

  • Good test coverage
  • Lean

Cons:

  • No explicit license, prefer 0.2.x.

Full Audit

negative
2019-09-11
MaulingMonkey
medium, low

A low-boilerplate, high performance archetype based ECS.

Pros:

  • Archetypes
  • Low boilerplate
  • MIT Licensed

Cons:

  • Widespread use of 16-bit values is begging for overflows
  • Some O(N) patterns I dislike, fortunately with small C.
  • Not MIRI friendly in even the most trivial of examples.
  • Difficult-to-vet unsafe code, strongly suspect some unsound.
  • Omnipresent low-value logging
  • Tons of dependencies (89)

0.1.1

crev
thoroughnessmedium
understandinglow (thanks to use of unsafe)
ratingnegative
FileRatingNotes
benches/allocation_stress.rs+1No global side effects, no black box use...?
benches/pos_vel.rs+1No global side effects, no black box use...?
examples/hello_world.rs+1
src/borrows.rs+1API lock design concerns me... and I suspect this relies on stable_drop_order to avoid aliasing violations.
src/lib.rs-1Difficult to vet unsafe. Some O(N) and overflow concerns.
src/query.rs-1Difficult to vet unsafe.
src/storage.rs-1Tons of difficult-to-vet UnsafeCell use that leaks some of it's unsafety out to other files.
tests/query_api.rs+1
tests/world_api.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+1caching cargo might be a bad idea IME
bench.png+1
Cargo.toml+1MIT
Cargo.toml.orig+1MIT
LICENSE+1MIT
readme.md+1
OtherRatingNotes
unsafe-1Lots of hard to reason about unsafe.
miri-1Trivial use chokes up miri.
fs+1
io+1
docs+1
tests+1

src/borrows.rs

LineWhatNotes
17Borrow::aquire_readPossible race condition source. Attempts to increment if >= 0. Theoretically could livelock if sufficiently contested, shouldn't in practice.
33Borrow::aquire_writePossible race condition source. Attempts to go from 0 => -1.
43Drop for Borrow+1
57BorrowedSafe from aliasing violations by virtue of stable_drop_order ? Cannot drop Borrow before value lifetime has ended.
92PartialEq for Borrowed'b lifetime unused...?
98Eq for Borrowed'b lifetime unused...?
128BorrowedMutSafe from aliasing violations by virtue of stable_drop_order ? Cannot drop Borrow before value lifetime has ended.
177BorrowedSliceSafe from aliasing violations by virtue of stable_drop_order ? Cannot drop Borrow before value lifetime has ended.
220BorrowedMutSliceSafe from aliasing violations by virtue of stable_drop_order ? Cannot drop Borrow before value lifetime has ended.
269BorrowedIterSafe from aliasing violations by virtue of stable_drop_order ? Cannot drop Borrow before value lifetime has ended.
EOF

src/lib.rs

LineWhatNotes
1Lib doc comments+1
254WorldIdOnly 16-bit world IDs? I could see this overflowing in practice, especially if using the advertized ability to stream stuff in.
264ArchetypeIdOnly 16-bit chunk IDs? Also, poor alignment.
281EntityStandard dual generation index, I approve... although hot entity IDs could overflow in practice?
310Universe::loggerNot entirely sure I'm a fan of this.
365ComponentIndex & friendsThese Indexes should be used earlier when defining WorldId etc....
409impl EntityBlock+1
431EntityBlock::in_rangeSlightly bogus u32 cast - EntityBlock::new should enforce u32 size if you want a u32 len...
498impl EntityAllocatorFull of O(N/1024) operations that could be O(1). Chunking is fine, but this really should abuse the fact that BLOCK_SIZE is constant more instead of looping.
517EntityAllocator::create_entityA better allocation strategy (IMO) would be to always start with the block we last allocated from. Will have degenerate behavior if we're constantly freeing/allocating from the first block.
625World::merge doc commentConcerning, implies API is unsound
638World::mergeunsafe { ... } - I think this might be sound thanks to &mut self implying we have exclusive access to self.archetypes[?].chunks()[?].entities()
719World::insertunsafe { ... } - I think this might be sound thanks to &mut self implying we have exclusive access to self.archetypes[?].chunks()[?].entities()
720World::insertunsafe { ... } - I think this might be sound thanks to &mut self implying we have exclusive access to self.archetypes[?].chunks()[?].entities()
805World::entity_data_mutunsafe { ... } - I think this might be sound thanks to &mut self implying we have exclusive access to self.archetypes[?].chunks()[?].entities()
841World::prep_archetypeunsafe { ... } - Probably sound - index should be valid - but why not just `.map(
1016EntitySource for IterEntitySourceunsafe { ... } - I think this might be sound thanks to &mut Chunk implying we have exclusive access to chunk.entities()
EOF

src/query.rs

LineWhatNotes
*PhantomDataFrequently used when T is already used in the struct, I'm concerned I'm missing something.
196impl View for (...) validate()unsafe { ... } - 0 <= i < j < types.len(), so both calls to get_unchecked should be sound.
276std::ops::Not for PassthroughOooh, operator overloading abuse. Probably fine?
1065impl Iterator for ZipEntities nextunsafe { ... } - Sketchy, assumes data.len() <= entities.len() and that's not a particularly trivial assertion to prove.
1088ChunkView::entitiesunsafe { ... } - NFI if this is sound or not.
EOF

src/storage.rs

LineWhatNotes
19StorageVecUnsafeCell! Oh no.
39unsafe fn StorageVec::data_mut(&self)Because this doesn't take &mut self, this forces the caller to enforce borrow checking manually.
47ComponentStorage::remove for StorageVecunsafe { ... } - I think this might be sound thanks to &mut self
52ComponentStorage::len for StorageVecunsafe { ... } - Unsound? It's not clear what, if anything, ensures len() isn't being mutated by another borrower.
104unsafe fn Chunk::entities_uncheckedBecause this doesn't take &mut self, this forces the caller to enforce borrow checking manually.
126unsafe fn Chunk::entity_data_unchecked+1?
141unsafe fn Chunk::entity_data_mut_uncheckedDitto.
157Chunk::entity_dataunsafe { ... } - Unsound? "Locks" via borrow types after constructing &T, which is too late.
174Chunk::entity_data_mutunsafe { ... } - Unsound? "Locks" via borrow types after constructing &mut T, which is too late.
199Chunk::shared_dataunsafe { ... } - NFI if this is sound or not. Haven't groked why SharedComponentStore is UnsafeCell or what protects it, if anything.
212Chunk::removeunsafe { ... } - I think this might be sound thanks to &mut self
EOF

Miri Example

use legion::prelude::*;

[test] // PREPEND YOUR OWN POUND SIGN
fn test() {
    let universe = Universe::new(None);
    let mut world = universe.create_world();
    world.insert_from((), vec![(1i32,)]);
}
rustup toolchain install nightly-2019-09-11
rustup component add miri --toolchain=nightly-2019-09-11
cargo +nightly-2019-09-11 miri test
error[E0080]: Miri evaluation error: trying to reborrow for SharedReadWrite, but parent tag <126591> does not have an appropriate item in the borrow stack
    |
    = note: inside call to `<legion::IterEntitySource<std::vec::IntoIter<(i32,)>, (i32,)> as legion::EntitySource>::write` at C:\Users\Mike\.cargo\registry\src\github.com-1ecc6299db9ec823\legion-0.1.1\src\lib.rs:716:29
    = note: inside call to `legion::World::insert::<(), legion::IterEntitySource<std::vec::IntoIter<(i32,)>, (i32,)>>` at C:\Users\Mike\.cargo\registry\src\github.com-1ecc6299db9ec823\legion-0.1.1\src\lib.rs:689:9
note: inside call to `legion::World::insert_from::<(), std::vec::Vec<(i32,)>>` at src\lib.rs:7:5
    --> src\lib.rs:7:5
     |
7    |     world.insert_from((), vec![(1i32,)]);
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside call to `test` at src\lib.rs:4:1
    --> src\lib.rs:4:1
     |
4    | / fn test() {
5    | |     let universe = Universe::new(None);
6    | |     let mut world = universe.create_world();
7    | |     world.insert_from((), vec![(1i32,)]);
8    | | }
     | |_^

TIL

  • TypeId::of::<T>()
positive
2019-09-05
MaulingMonkey
medium, medium

Utility crate... LGTM!

positive
2019-09-05
MaulingMonkey
medium, medium

Internal implementation detail crate of macro_rules_attribute. Relatively trivial... LGTM.

positive
2019-07-23
MaulingMonkey
medium, medium

Trivial, looks correct, looks tested. cfg!(debug_assertions) is a sane cfg.

negative
2019-08-30
MaulingMonkey
medium, medium

API design is super brittle. Returning uninitialized memory seems like UB-bait.

Detail

FileRatingNotes
src/lib.rs-1Soundish, but unsafe as heck APIs.
.cargo-ok+1
.gitignore+1
.travis.yml+1
Cargo.toml+1
README.md+1
OtherRatingNotes
unsafe-1Soundish, but unsafe as heck API design.
fs+1None
io+1None
docs+1
tests+1

src/lib.rs

LineWhatNotes
23allocateThis looks like it returns uninitialized memory (only sizes capacity not actual size). UB bait. Unaligned.
38reallocateMinimal debug checks, constructs Vec from_raw_parts with size when the real vec had size 0.
negative
2020-01-09
MaulingMonkey
low, low

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/microprofile.md#021
Decent seeming crate, but 98% underdocumented underfuzzed C++ that is network exposed, parses strings, rewrites x86, etc.
Might use for development but using it in production would just be begging for CVEs.

negative
2019-09-02
MaulingMonkey
low, low

Good starting point, and my favorite Rust MIDI API so far, but likely unsound as stands.

Pros:

  • Pure rust, no mucking with building building C/C++ libs like with portmidi
  • To be WASM/Browser compatible (PR made).

Cons:

  • Likely unsound as currently stands (transmute_copy, type punning, sketchy memalloc crate use, haven't vetted thread safety properly, ...)
  • API clunky in spots (mix of known issues and easily fixed surface level stuff)
  • Needs more unit test coverage if possible (are there perhaps virtual MIDI devices for windows that could be added to CI...?)

TODO:

  • rating: netural: Eliminate as much sketchy unsafe as possible.
  • rating: positive: Clean up API design a bit (async, saner member functions, deal with Send inconsistency, maybe make connecting not consume MidiInput s?)

Detail

FileRatingNotes
examples/test_forward.rs+1
examples/test_play.rs+1
examples/test_reuse.rs+1
examples/test_sysex.rs+1
src/backend/asla/mod.rs0Some extra unsafe, use of uninit data possibly UB
src/backend/coremidi/mod.rs0Is Core MIDI thread safe? (unsafe impl Send for MidiOutputConnection)
src/jack/mod.rs-1Use of transmute_copy on Box is skeeeeeeetchy, uninitialized too :(
src/jack/wrappers.rs-1Is JACK thread safe? Ringbuffer::read is unsound! Lots of unsafe but mostly for sane FFI.
src/winmm/handler.rs0unsafe for FFI, some pointer casts I haven't thoroughly vetted
src/winmm/mod.rs0unsafe for FFI, uninitialized :(, sketchy deallocate API. Is WinMM thread safe? 333: UB &mut violates aliasing?
src/backend/mod.rs+1
src/os/mod.rs+1
src/os/unix.rs+1
src/common.rs0Consumption of the MidiInput/MidiOutput clients/factories on connect seems a bit strange, especially after the ports refactor.
src/errors.rs+1
src/lib.rs+1unsafe, but sound
tests/virtual.rs+1
.cargo-ok+1
.gitignore+1
.travis.yml+1RCE: Downloads/installs jack from the internet
appveyor.yml+1
Cargo.toml+1
Cargo.toml.orig+1
LICENSE+1
README.md+1
OtherRatingNotes
unsafe-199% sure something in here is unsound. transmute_copy, type punning, use of memalloc crate.
fs+1N/A
io0libc... maybe safe?
docs+1
tests0Good examples, but needs more automated unit/integration tests. Admittedly hard given the lack of MIDI devices on CI servers...

src/backend/asla/mod.rs

LineWhatNotes
21fn poll+1
81unsafe impl Send for EventEncoderIs ASLA thread safe?
578unsafe in handle_inputConstruction of uninitialized vec, UB?
positive
2020-10-11
MaulingMonkey
high, high

[Full Audit]
std::num::NonZero___ equivalents

Pros:

  • Works
  • Can represent 0
  • Clever cheap xors for implementation

Cons:

  • Missing a lot of traits vs equivalent NonZero*s
  • Extra pointless unsafe in new impl
positive
2020-10-11
MaulingMonkey
high, high

[Full Audit]
std::num::NonZero___ equivalents

Pros:

  • Works
  • Can represent 0
  • Clever cheap xors for implementation

Cons:

  • Missing a lot of traits vs equivalent NonZero*s
  • Extra pointless unsafe in new impl
positive
2020-10-11
MaulingMonkey
high, high

[Full Audit]
std::num::NonZero___ equivalents

Pros:

  • Works
  • Can represent 0
  • Clever cheap xors for implementation

Cons:

  • Missing a lot of traits vs equivalent NonZero*s
  • Extra pointless unsafe in new impl
positive
2020-09-07
MaulingMonkey
low, low

Queries the OS for the number of CPU cores you have.

Pros:

  • You didn't have to write it.
  • Handles all that hideously platform specific nonsense for you.

Cons:

  • Lots of (necessary) unsafe
  • Linux cgroups support seems wildly overcomplicated.
  • Lies and
    lies and
    lies and
    lies.
    Not exactly this crate's fault - the system APIs are brittle and full of edge cases.
    Multi-processor architectures are full of edge cases.

Alternatives:

  • Just hardcode a reasonable number of threads for your workload!
    Spinning up threads for 64 CPU cores to all false-share a single cacheline
    because that's how many logical cores were detected isn't the right choice!
    And all your threads are probably blocked on I/O anyways!

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium

Queries the OS for the number of CPU cores you have.

Pros:

  • You didn't have to write it.
  • Handles all that hideously platform specific nonsense for you.

Cons:

  • Lots of (necessary) unsafe
  • Linux cgroups support seems wildly overcomplicated.
  • Lies and
    lies and
    lies and
    lies.
    Not exactly this crate's fault - the system APIs are brittle and full of edge cases.
    Multi-processor architectures are full of edge cases.

Alternatives:

  • Just hardcode a reasonable number of threads for your workload!
    Spinning up threads for 64 CPU cores to all false-share a single cacheline
    because that's how many logical cores were detected isn't the right choice!
    And all your threads are probably blocked on I/O anyways!

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium
advisories:
medium

Queries the OS for the number of CPU cores you have.

Pros:

  • You didn't have to write it.
  • Handles all that hideously platform specific nonsense for you.

Cons:

  • Lots of (necessary) unsafe
  • Linux cgroups support seems wildly overcomplicated.
  • Lies and
    lies and
    lies and
    lies.
    Not exactly this crate's fault - the system APIs are brittle and full of edge cases.
    Multi-processor architectures are full of edge cases.

Alternatives:

  • Just hardcode a reasonable number of threads for your workload!
    Spinning up threads for 64 CPU cores to all false-share a single cacheline
    because that's how many logical cores were detected isn't the right choice!
    And all your threads are probably blocked on I/O anyways!

Full Audit

positive
2020-03-21
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.4.3

positive
2020-03-21
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.4.1

positive
2020-03-21
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.4.0

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.3.1

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.3.0

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.2.3

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.2.2

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.2.1

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.2.0

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.1.1

positive
2020-03-20
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum.md#0.1.0

positive
2020-03-21
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum_derive.md#0.4.3

positive
2020-03-21
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum_derive.md#0.4.2

positive
2020-03-21
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum_derive.md#0.4.1

positive
2020-03-21
MaulingMonkey
low, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum_derive.md#0.4.0

strong
2020-03-21
MaulingMonkey
high, high

Placeholder
https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/num_enum_derive.md#0.0.0

positive
2019-07-31
MaulingMonkey
low, medium

Solid looking, 100% safe. Only concern is CI scripts downloading random URLs.

positive
2020-01-08
MaulingMonkey
medium, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/proc-macro-crate.md#014

positive
2020-01-08
MaulingMonkey
medium, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/proc-macro-crate.md#013

positive
2020-01-08
MaulingMonkey
medium, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/proc-macro-crate.md#012

positive
2020-01-08
MaulingMonkey
medium, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/proc-macro-crate.md#011

positive
2020-01-08
MaulingMonkey
medium, medium

https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/proc-macro-crate.md#010

neutral
2019-07-25
MaulingMonkey
medium, medium

Sound, although buggy AMD hardware makes implementing CryptoRng for RdRand/RdSeed a little unnerving to me ( https://github.com/nagisa/rust_rdrand/issues/12 )

Reviewed:
benches\rdrand.rs: +1
benches\rdseed.rs: +1
benches\std.rs: +1
src\changelog.rs: +1
src\lib.rs: 0
All of $gen::try_fill_bytes::imp being unsafe is still concerning... but I believe it's sound now.
RdRand/RdSeed implement CrytoRng, which makes buggy AMD hardware concerning: https://github.com/nagisa/rust_rdrand/issues/12

Concerns reduced since 0.4.0:
https://github.com/nagisa/rust_rdrand/commit/7af432c6e315fde053d0056d1b7df893a865711a
unsafe blocks appear much larger than they need to be.

Concerns fixed since 0.4.0:
https://github.com/nagisa/rust_rdrand/commit/26a0a2f9d885fbbb8e14fa47c8a48e366cf15455
mem::uninitialized() u32s
loop_rand! uses mem::uninitialized() for $el:ty, easy to misuse! Requires unsafe{} so technically sound. Not exported.
https://github.com/nagisa/rust_rdrand/commit/7af432c6e315fde053d0056d1b7df893a865711a
$gen::try_fill_bytes: UNSOUND! word and buffer reference the same data. As both are &mut Ts, this is 100% undefined behavior.
ptr::copy_nonoverlapping: This should really use a slice copy which should be just as safe...? But maybe missing from core?
Verified vs https://www.amd.com/system/files/TechDocs/24594.pdf
is_x86_feature_detected: I have not verified this is correct.

neutral
2019-07-25
MaulingMonkey
medium, medium

Sound, although buggy AMD hardware makes implementing CryptoRng for RdRand/RdSeed a little unnerving to me ( https://github.com/nagisa/rust_rdrand/issues/12 )

Reviewed:
benches\rdrand.rs: +1
benches\rdseed.rs: +1
benches\std.rs: +1
src\changelog.rs: +1
src\lib.rs: 0
All of $gen::try_fill_bytes::imp being unsafe is still concerning... but I believe it's sound now.
RdRand/RdSeed implement CrytoRng, which makes buggy AMD hardware concerning: https://github.com/nagisa/rust_rdrand/issues/12

Concerns reduced since 0.4.0:
https://github.com/nagisa/rust_rdrand/commit/7af432c6e315fde053d0056d1b7df893a865711a
unsafe blocks appear much larger than they need to be.

Concerns fixed since 0.4.0:
https://github.com/nagisa/rust_rdrand/commit/26a0a2f9d885fbbb8e14fa47c8a48e366cf15455
mem::uninitialized() u32s
loop_rand! uses mem::uninitialized() for $el:ty, easy to misuse! Requires unsafe{} so technically sound. Not exported.
https://github.com/nagisa/rust_rdrand/commit/7af432c6e315fde053d0056d1b7df893a865711a
$gen::try_fill_bytes: UNSOUND! word and buffer reference the same data. As both are &mut Ts, this is 100% undefined behavior.
ptr::copy_nonoverlapping: This should really use a slice copy which should be just as safe...? But maybe missing from core?
Verified vs https://www.amd.com/system/files/TechDocs/24594.pdf
is_x86_feature_detected: I have not verified this is correct.

negative
2019-07-24
MaulingMonkey
medium, medium

$gen::try_fill_bytes invokes undefined behavior (overlapping &mut u32 and &mut [u8]).

Reviewed:
benches\rdrand.rs: +1
benches\rdseed.rs: +1
benches\std.rs: +1
src\changelog.rs: +1
src\lib.rs: Concerns:
mem::uninitialized() u32s
is_x86_feature_detected: I have not verified this is correct.
loop_rand! uses mem::uninitialized() for $el:ty, easy to misuse! Requires unsafe{} so technically sound. Not exported.
$gen::try_fill_bytes: UNSOUND! word and buffer reference the same data. As both are &mut Ts, this is 100% undefined behavior.
unsafe blocks appear much larger than they need to be.
ptr::copy_nonoverlapping: This should really use a slice copy which should be just as safe...? But maybe missing from core?

negative
2019-07-24
MaulingMonkey
medium, medium

$gen::try_fill_bytes invokes undefined behavior (overlapping &mut u32 and &mut [u8]): https://github.com/nagisa/rust_rdrand/issues/13. 0.5.x removed some use of uninitialized.

Reviewed:
benches\rdrand.rs: +1
benches\rdseed.rs: +1
benches\std.rs: +1
src\changelog.rs: +1
src\lib.rs: Concerns:
mem::uninitialized() u32s
is_x86_feature_detected: I have not verified this is correct.
loop_rand! uses mem::uninitialized() for $el:ty, easy to misuse! Requires unsafe{} so technically sound. Not exported.
$gen::try_fill_bytes: UNSOUND! word and buffer reference the same data. As both are &mut Ts, this is 100% undefined behavior.
unsafe blocks appear much larger than they need to be.
ptr::copy_nonoverlapping: This should really use a slice copy which should be just as safe...? But maybe missing from core?

positive
2020-09-11
MaulingMonkey
high, high
alternative:
Less generalized alternative, but implements read_at/write_at on &self for File by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • Very thorough, well documented

Cons:

  • File isn't ReadAt/WriteAt on windows due to seeks... reasonable but annoying
  • A couple of minor API holes to be fixed
positive
2020-09-11
MaulingMonkey
high, high
alternative:
Less generalized alternative, but implements read_at/write_at on &self for File by default

Offset read/write with &self

Pros:

  • No runtime dependencies
  • Very thorough, well documented

Cons:

  • File isn't ReadAt/WriteAt on windows due to seeks... reasonable but annoying
  • A couple of minor API holes to be fixed

Full Audit

negative
2019-07-24
MaulingMonkey
low, low

Exposes unsound APIs, lots of unverified syscalls.

Reviewed:

src\arch*.rs: Skimmed... looks reasonable, but didn't verify correct instructions / register invalidation.

src\io\dma.rs: Some unsafe... looks correct, but not thoroughly tested.
src\io\io.rs: +1
src\io\mmio.rs: UNSOUND (can construct uninitialized() T via "safe" Mmio::new()!)
src\io\mod.rs: +1
src\io\pio.rs: Some unsafe... looks reasonable, but didn't verify correct instructions.

src\scheme\generate.sh: +1
src\scheme\mod.rs: +1
src\scheme\scheme*.rs: UNSOUND (can construct arbitrary slices from arbitrary Packet s via Scheme*::handle)

.cargo_vcs_info.json: +1
.cargo-ok: +1
.gitlab-ci.yml: +1
Cargo.toml: +1
Cargo.toml.orig: +1
LICENSE: +1
README.md: +1

Skimmed:

src\call.rs: Lots of unsafe syscalls... unverified.
src\data.rs: UNSOUND (Map deref etc.)
src\error.rs: No tests for STR_ERROR, but at least it's sound.
src\flag.rs: Sound, magic constant city, meh.
src\lib.rs: LEAKS UNSOUND TRAITS into public interface!
src\number.rs: Safe, magic constant city.
src\tests.rs: Unsafe, but only #[test]s

positive
2019-09-03
MaulingMonkey
medium, low

0.2.0-alpha: Significant refactoring, adding support for generics. LGTM?
0.1.2: Seems solid, although my syn-fu is weak, limiting my ability to review this.

Detail

FileRatingNotes
src/docs/require_unsafe_in_bodies.md+1
src/docs/require_unsafe_in_body.md+1
src/utils/macros.rs+1
src/utils/mod.rs+1
src/lib.rs+1understanding: low - I'm barely following along
src/tests.rs+1
tests/ui/body_on_method_2.rs+1
tests/ui/body_on_method_2.stderr+1
tests/ui/body_on_method.rs+1
tests/ui/body_on_method.stderr+1
tests/ui/readme.rs+1
tests/ui/readme.stderr+1
tests/impl_method_2.rs+1The cfgs on unit-tests look the wrong way around?
tests/impl_method.rs+1The cfgs on unit-tests look the wrong way around?
tests/trait_default_method.rs+1The cfgs on unit-tests look the wrong way around?
tests/ui.rs+1The cfgs on unit-tests look the wrong way around?
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
Cargo.toml+1MIT
Cargo.toml.orig+1MIT
LICENSE+1MIT
Makefile+1Unixy
README.md+1
OtherRatingNotes
unsafe+1Wrangles unsafe but doesn't actually use it itself
fs+1None
io+1Modifies codegen through syn
docs+1
tests+1

TIL

let Struct { ref member, ref mut member2, .. } = to_destructure;
positive
2019-09-03
MaulingMonkey
low, low

0.2.0: More refactoring surrounding generics.
0.2.0-alpha: Significant refactoring, adding support for generics. LGTM?
0.1.2: Seems solid, although my syn-fu is weak, limiting my ability to review this.

Detail

FileRatingNotes
src/docs/require_unsafe_in_bodies.md+1
src/docs/require_unsafe_in_body.md+1
src/utils/macros.rs+1
src/utils/mod.rs+1
src/lib.rs+1understanding: low - I'm barely following along
src/tests.rs+1
tests/ui/body_on_method_2.rs+1
tests/ui/body_on_method_2.stderr+1
tests/ui/body_on_method.rs+1
tests/ui/body_on_method.stderr+1
tests/ui/readme.rs+1
tests/ui/readme.stderr+1
tests/impl_method_2.rs+1The cfgs on unit-tests look the wrong way around?
tests/impl_method.rs+1The cfgs on unit-tests look the wrong way around?
tests/trait_default_method.rs+1The cfgs on unit-tests look the wrong way around?
tests/ui.rs+1The cfgs on unit-tests look the wrong way around?
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
Cargo.toml+1MIT
Cargo.toml.orig+1MIT
LICENSE+1MIT
Makefile+1Unixy
README.md+1
OtherRatingNotes
unsafe+1Wrangles unsafe but doesn't actually use it itself
fs+1None
io+1Modifies codegen through syn
docs+1
tests+1

TIL

let Struct { ref member, ref mut member2, .. } = to_destructure;
positive
2019-08-21
MaulingMonkey
medium, low

Seems solid, although my syn-fu is weak, limiting my ability to review this.

Detail

FileRatingNotes
src/docs/require_unsafe_in_bodies.md+1
src/docs/require_unsafe_in_body.md+1
src/utils/macros.rs+1
src/utils/mod.rs+1
src/lib.rs+1understanding: low - I'm barely following along
src/tests.rs+1
tests/ui/body_on_method_2.rs+1
tests/ui/body_on_method_2.stderr+1
tests/ui/body_on_method.rs+1
tests/ui/body_on_method.stderr+1
tests/ui/readme.rs+1
tests/ui/readme.stderr+1
tests/impl_method_2.rs+1The cfgs on unit-tests look the wrong way around?
tests/impl_method.rs+1The cfgs on unit-tests look the wrong way around?
tests/trait_default_method.rs+1The cfgs on unit-tests look the wrong way around?
tests/ui.rs+1The cfgs on unit-tests look the wrong way around?
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
Cargo.toml+1MIT
Cargo.toml.orig+1MIT
LICENSE+1MIT
Makefile+1Unixy
README.md+1
OtherRatingNotes
unsafe+1Wrangles unsafe but doesn't actually use it itself
fs+1None
io+1Modifies codegen through syn
docs+1
tests+1

TIL

let Struct { ref member, ref mut member2, .. } = to_destructure;
positive
2019-12-27
MaulingMonkey
high, medium

Excellent looking crate.
Full notes: https://github.com/MaulingMonkey/rust-reviews/blob/master/reviews/rustversion.md#100

positive
2019-09-15
MaulingMonkey
medium, high

Pros:

  • 100% safe code.
  • 0 dependencies.
  • Does exactly what it's supposed to do.
  • Great test coverage.

Cons:

  • Might want to feature-gate functions which assume std::env access under the hood.
  • Env access (but that's the whole point)
  • No ~username/ support.
  • No %ENV% support (windows style env var syntax... or maybe that's a feature?).

1.0.0

  • Minor breaking changes
  • Typo fixes
  • Missing example fixes

0.1.0

FileRatingNotes
src/lib.rs+1
.cargo-ok+1
.gitignore+1
.travis.yml+1Installs travis-cargo
Cargo.lock+1
Cargo.toml+1No 3rd party deps
LICENSE-APACHE+1
LICENSE-MIT+1
Readme.md+1Properly dual licensed
OtherRatingNotes
unsafe+1None
miri-1"can't call foreign function: GetCurrentProcess"
fs+1None
io0Hardcoded env fns might need to be featured out for browser WASM targets
docs+1
tests+1Doc tests

src/lib.rs

LineWhatNotes
1lib.rs doc comments+1
155fn full_with_context+1
229fn full_with_context_no_errors+1
283fn full+1
295struct LookupError+1
303impl Display for LookupError+1
309impl Error for LookupError+1
314macro_rules try_lookup!+1
323fn is_valid_var_name_char+1 - includes unicode, as mentioned in docs
393fn env_with_context+1
506fn env_with_context_no_errors+1
552fn env+1
584fn tilde_with_context0 - example incomplete
633fn tilde+1
637mod tilde_tets+1
675mod env_test+1
821mod full_tests+1

TIL

Apparently you can use references link style for badge images. Huh! I should use that for reviews...

positive
2019-09-15
MaulingMonkey
medium, high

Pros:

  • 100% safe code.
  • 0 dependencies.
  • Does exactly what it's supposed to do.
  • Great test coverage.

Cons:

  • Might want to feature-gate functions which assume std::env access under the hood.
  • Env access (but that's the whole point)
  • No ~username/ support.
  • No %ENV% support (windows style env var syntax... or maybe that's a feature?).

0.1.0

FileRatingNotes
src/lib.rs+1
.cargo-ok+1
.gitignore+1
.travis.yml+1Installs travis-cargo
Cargo.lock+1
Cargo.toml+1No 3rd party deps
LICENSE-APACHE+1
LICENSE-MIT+1
Readme.md+1Properly dual licensed
OtherRatingNotes
unsafe+1None
miri-1"can't call foreign function: GetCurrentProcess"
fs+1None
io0Hardcoded env fns might need to be featured out for browser WASM targets
docs+1
tests+1Doc tests

src/lib.rs

LineWhatNotes
1lib.rs doc comments+1
155fn full_with_context+1
229fn full_with_context_no_errors+1
283fn full+1
295struct LookupError+1
303impl Display for LookupError+1
309impl Error for LookupError+1
314macro_rules try_lookup!+1
323fn is_valid_var_name_char+1 - includes unicode, as mentioned in docs
393fn env_with_context+1
506fn env_with_context_no_errors+1
552fn env+1
584fn tilde_with_context0 - example incomplete
633fn tilde+1
637mod tilde_tets+1
675mod env_test+1
821mod full_tests+1

TIL

Apparently you can use references link style for badge images. Huh! I should use that for reviews...

negative
2019-07-23
MaulingMonkey
none, none

Lots of unsafe code, long standing open 'maybe UB' issues that proved to be correct, 3 previous rustsec advisories, no obvious fuzz tests. Use only under duress.

positive
2019-07-23
MaulingMonkey
medium, medium

Slightly odd-seeming design, looks sane/safe though.

neutral
2019-09-19
MaulingMonkey
medium, medium

Pros:

  • Very well documented
  • Well tested
  • Good attention to detail WRT unsafe code.

Cons:

  • Issue #644 Scary partially uninitialized Vec<T>s
  • PR #645println!()s for !is_unconstrained() joins is lame / a perf hazard in and of itself.
  • Complicated as heck
  • I'm not yet convinced the ParJoin s are sound
  • Plenty of unsafe code
    • Some scary lifetime extensions
    • Comments like "// This is horribly unsafe."
  • See -1s for more details

0.15.1

Misc. notes:

  • I still don't have a firm grasp on when world.maintain() is necessary
crev
thoroughnessmedium
understandingmedium
ratingneutral
FileRatingNotes
.github/ISSUE_TEMPLATE/bug_report.md+1
.github/ISSUE_TEMPLATE/feature_request.md+1
.github/stale.yml+1
benches/benches_main.rs+1criterion
benches/big_or_small.rs+1nalgebra, shred
benches/parallel.rs+1
benches/storage_cmp.rs+1
benches/storage_sparse.rs+1
benches/world.rs+1rayon
docs/reference/src/01_system.md0~empty
docs/reference/src/intro.md+1
docs/reference/src/SUMMARY.md+1TOC
docs/reference/book.toml+1
docs/tutorials/src/images/component-tables.svg+1<switch>? <foreignObject>? What is this madness...
docs/tutorials/src/images/entity-component.svg+1
docs/tutorials/src/images/system.svg+1
docs/tutorials/src/01_intro.md+1Java?
docs/tutorials/src/02_hello_world.md0specs 0.15.0, lots of rust,ignores, poor link names
docs/tutorials/src/03_dispatcher.md+1
docs/tutorials/src/04_resources.md+1Option<Read<...>>
docs/tutorials/src/05_storages.md+1
docs/tutorials/src/06_system_data.md+1
docs/tutorials/src/07_setup.md+1
docs/tutorials/src/08_join.md+1
docs/tutorials/src/09_parallel_join.md+1
docs/tutorials/src/10_rendering.md+1
docs/tutorials/src/11_advanced_component.md+1
docs/tutorials/src/12_tracked.md+1
docs/tutorials/src/13_saveload.md+1
docs/tutorials/src/14_troubleshooting.md+1
docs/tutorials/src/SUMMARY.md+1TOC
docs/tutorials/book.toml+1
docs/website/content/pages/about.md+1
docs/website/content/pages/docs.md+1
docs/website/content/_index.md+1
docs/website/content/specs-0.15.md+1draft, "Test"
docs/website/themes/hyde/sass/hyde.scss+1
docs/website/themes/hyde/sass/poole.scss+1
docs/website/themes/hyde/sass/print.scss+1
docs/website/themes/hyde/static/.gitkeep+1
docs/website/themes/hyde/templates/404.html+1some templating engine used
docs/website/themes/hyde/templates/index.html+1
docs/website/themes/hyde/templates/page-nodate.html+1
docs/website/themes/hyde/templates/page.html+1
docs/website/themes/hyde/.gitignore+1
docs/website/themes/hyde/config.toml+1
docs/website/themes/hyde/LICENSE+1MIT
docs/website/themes/hyde/README.md+1
docs/website/themes/hyde/theme.toml+1
docs/website/config.toml+1
examples/async.rs+1
examples/basic.rs+1
examples/bitset.rs+1fizzbuzz! Hah.
examples/cluster_bomb.rs+1
examples/full.rs+1dispatcher.with_barrier()
examples/ordered_track.rs+1
examples/saveload.rs+1ron, structs/impls inside fns
examples/track.rs+1
scripts/build-netlify.sh0Installing things from the internet (zola, mdbook, rustup)
scripts/kcov.sh-1docker - owns/deletes mykcov1, runs images, disables seccomp
src/join/mod.rs0I don't fully grok the ParJoin limitations yet, nor the underlying reason behind unsafe fn open/get.
src/join/par_join.rs-1"[...] technically not allowed" (ab)use of UnsafeCell
src/saveload/de.rs+1
src/saveload/marker.rs+1
src/saveload/mod.rs+1
src/saveload/ser.rs+1
src/saveload/tests.rs+1
src/saveload/uuid.rs+1
src/storage/data.rs+1
src/storage/drain.rs+1
src/storage/entry.rs-1Entries::get seems sketchy
src/storage/flagged.rs+1
src/storage/generic.rs+1
src/storage/mod.rs-1&mut Storage::get_mut is sketchy as heck
src/storage/restrict.rs-1More ParJoin I can't wrap my head around, _unchecked fns sound unsafe but aren't marked as such.
src/storage/storages.rs-1#644, UnprotectedStorage is a scary API in general for perf reasons.
src/storage/tests.rs+1needs more
src/storage/track.rs0
src/world/comp.rs+1
src/world/entity.rs0Possibly missing some self.killed.contains(...) checks in Allocator, ungrokked ParJoin
src/world/lazy.rs+1
src/world/mod.rs+1
src/world/tests.rs+1
src/world/world_ext.rs0Some concerns about create_entity_unchecked, is_alive
src/bitset.rs+1
src/changeset.rs-1Bypasses lifetime checks
src/error.rs+1
src/lib.rs+1
src/prelude.rs+1
tests/saveload.rs+1
tests/tests.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.rustfmt.toml+1
Cargo.lock+1
Cargo.toml+1
Cargo.toml.orig+1
CHANGELOG.md+1I like the link format here
CODE_OF_CONDUCT.md+1
codecov.yml+1
CONTRIBUTING.md+1
LICENSE-APACHE+1
LICENSE-MIT+1
netlify.toml+1
PULL_REQUEST_TEMPLATE.md+1
README.md+1Dual MIT/Apache 2.0, could use a deps badge
OtherRatingNotes
unsafe-1ParJoin sounds unsound, scary comments like "// This is horribly unsafe."
miri-1N/A (atomics)
fs+1N/A
io+1N/A
docs+1Sets the golden standard for how to document unsafe code in many places... but sadly not all.
tests0Some gaps on a per-module basis, which really seem necessary for some of the scarier nests of unsafe code.

src/join/mod.rs

LineWhatNotes
227unsafe fn Join::openHmm... will this truly be unsound? Will need to read more...
237unsafe fn Join::get"The implementation [...] has no invariants to meet" - I don't grok why this is unsafe yet, but tat least the doc comments seem clear.
273unsafe fn MaybeJoin::open+1, just forwards, good doc comments about held constraints
280unsafe fn MaybeJoin::get+1, meets documented constraints, good doc comments about held constraints
305fn JoinIter::new0, fix pending: #645 println! isn't an appropriate mechanism for this and possibly adds another perf issue. PR switches to log crate.
312fn JoinIter::new0, relies on constraint of impl
370fn JoinIter::get+1, documents and abides by safety constraints
386fn JoinIter::get_unchecked+1, documents and abides by safety constraints
399fn JoinIter::next+1, documents and abides by safety constraints
422unsafe fn (A,B,...)::open+1, documents and abides by safety constraints
434unsafe fn (A,B,...)::get+1, documents and abides by safety constraints
440fn (A,B,...)::is_unconstrained+1
451unsafe impl ParJoin for (A,B,...)0, sounds accurate but I don't fully grok
500unsafe fn _Readers_::open+1, just forwards sanely
506unsafe fn _Readers_::get+1, just forwards sanely
511unsafe fn _Readers_::is_unconstrained+1
519unsafe impl ParJoin for _Readers_0, sounds accurate but I don't fully grok
542unsafe fn _Writers_::open+1, just forwards sanely
548unsafe fn _Writers_::get+1, just forwards sanely
553unsafe fn _Writers_::is_unconstrained+1
561unsafe impl ParJoin for _Writers_0, sounds accurate but I don't fully grok
EOF

src/join/par_join.rs

LineWhatNotes
21fn ParJoin::par_join comment-1, "NOTE: This is currently unspecified behavior." - concerning...
30fn ParJoin::par_join0, fix pending: #645 println! isn't an appropriate mechanism for this and possibly adds another perf issue. PR switches to log crate.
56fn JoinParIter::drive_unindexed0, unsafe { ... } relies on constraint of impl
98unsafe impl Send for JoinProducer-1, "[...] technically not allowed" abuse of UnsafeCell
130fn UnindexedProducer::fold_with0, logic makes sense to me here
EOF

src/storage/drain.rs

LineWhatNotes
25unsafe fn Drain::open+1, just forwards sanely
32unsafe fn Drain::get+1, uses remove sanely
EOF

src/storage/entry.rs

LineWhatNotes
41fn Storage::entry+1, unsafe { ... } documents and abides by safety constraints
121fn Storage::entries+1
140unsafe fn Entries::open+1
146unsafe fn Entries::get-1, unsafe { ... } - I can't verify how this lifetime extension is possibly sound either.
164fn Entries::is_unconstrained+1
184fn OccupiedEntry::get+1, unsafe { ... } - documents and abides by safety constraints
197fn OccupiedEntry::get_mut+1, unsafe { ... } - documents and abides by safety constraints
205fn OccupiedEntry::into_mut+1, unsafe { ... } - documents and abides by safety constraints
209fn OccupiedEntry::insert+1
215fn OccupiedEntry::remove+1
236fn VacantEntry::insert+1, unsafe { ... } - documents and abides by safety constraints
EOF

src/storage/flagged.rs

LineWhatNotes
203unsafe fn FlaggedStorage::clean+1, just forwards sanely
210unsafe fn FlaggedStorage::get+1, just forwards sanely
214unsafe fn FlaggedStorage::get_mut+1, just forwards sanely
221unsafe fn FlaggedStorage::insert+1, just forwards sanely
228unsafe fn FlaggedStorage::remove+1, just forwards sanely
EOF

src/storage/mod.rs

LineWhatNotes
57unsafe fn AntiStorage::open+1, safe and sound
62unsafe fn AntiStorage::get+1, safe and sound
66unsafe impl DistinctStorage for AntiStorage+1, safe and sound
70unsafe impl ParJoin for AntiStorage+1, safe and sound
78unsafe impl CastFrom for dyn AnyStorage0, don't fully grok why this is unsafe, but looks 100% safe and sound and matches shred examples.
122unsafe trait DistinctStorage+1, safe and sound
155fn MaskedStorage::clear+1, unsafe { ... } - documents and abides by safety constraints
165fn MaskedStorage::remove+1, unsafe { ... } - documents and abides by safety constraints
175fn MaskedStorage::drop+1, unsafe { ... } - documents and abides by safety constraints
231fn Storage::get+1, unsafe { ... } - documents and abides by safety constraints
274unsafe fn Storage::unprotected_storage_mut+1, documents constraints
282fn Storage::get_mut+1, unsafe { ... } - documents and abides by safety constraints
299fn Storage::insert+1, unsafe { ... } - documents and abides by safety constraints
304fn Storage::insert+1, unsafe { ... } - documents and abides by safety constraints
341unsafe impl DistinctStorage for Storage+1, proper constraints
356unsafe fn &Storage::open+1, documents constraints
362unsafe fn &Storage::get+1, just forwards sanely
400unsafe fn &mut Storage::open+1, documents constraints
409unsafe fn &mut Storage::get-1, WTF? Lifetime extension? Can't this just be v.get_mut(i)?
449trait UnprotectedStorage-1, needs to make it clearer that clearmust be called before dropping as currently stands, unless that's just a bug.
457unsafe fn UnprotectedStorage::clear+1, documents constraints
472unsafe fn UnprotectedStorage::get+1, documents constraints
485unsafe fn UnprotectedStorage::get_mut+1, documents constraints
496unsafe fn UnprotectedStorage::insert+1, documents constraints
504unsafe fn UnprotectedStorage::remove+1, documents constraints
514unsafe fn UnprotectedStorage::drop+1, documents constraints, default impl sane/sound
EOF

src/storage/restrict.rs

LineWhatNotes
83unsafe impl ParJoin for &mut RestrictedStorage<..., MutableParallelRestriction>-1, no idea
93unsafe impl ParJoin for &RestrictedStorage<..., ImmutableAliasing>-1, no idea
113unsafe fn &RestrictedStorage::open0, could maybe document safety more, but I believe sound?
118unsafe fn &RestrictedStorage::get0, could maybe document safety more, but I believe sound?
144unsafe fn &mut RestrictedStorage::open0, could maybe document safety more, but I believe sound?
149unsafe fn &mut RestrictedStorage::get0, could maybe document safety more, but I believe sound?
239fn PairedStorage::get_unchecked-1, unsafe { ... } - maybe fine, but safety undocumented and get_unchecked sounds like something that should itself be an unsafe fn?
252fn PariedStorage::get_mut_unchecked-1, unsafe { ... } - maybe fine, but safety undocumented and get_unchecked sounds like something that should itself be an unsafe fn?
272fn PairedStorage::get+1, abides by safety constraints
294fn PairedStorage::get_mut+1, abides by safety constraints
EOF

src/storage/storages.rs

Careful auditing gives a good idea for what the actual API requirements of UnprotectedStorage are, and it's a doozy.

  1. VecStorage makes it unsound to not call clean
  2. Noop cleans on several storages make me think it should consume self instead of taking &mut self - maybe can't due to API limitations?
  3. Need clearer UnprotectedStorage docs
LineWhatNotes
20unsafe fn BTreeStorage::clean0, should maybe clear storage?
27unsafe fn BTreeStorage::get+1
31unsafe fn BTreeStorage::get_mut+1
35unsafe fn BTreeStorage::insert+1
39unsafe fn BTreeStorage::remove+1
44unsafe impl DistinctStorage for BTreeStorage+1 as I grok DistinctStorage
54unsafe fn HashMapStorage::clean0, should maybe clear storage?
61unsafe fn HashMapStorage::get+1
65unsafe fn HashMapStorage::get_mut+1
69unsafe fn HashMapStorage::insert+1
73unsafe fn HashMapStorage::remove+1
78unsafe impl DistinctStorage for HashMapStorage+1
95unsafe fn DenseVecStorage::clean0, should maybe clear storage?
102unsafe fn DenseVecStorage::get0, unsound on bad index, but fn is unsafe. Otherwise sound.
107unsafe fn DenseVecStorage::get_mut0, unsound on bad index, but fn is unsafe. Otherwise sound.
112unsafe fn DenseVecStorage::insert0, set_len leaves uninitialized gaps in the data_id Vec. Maybe sound, but at least violates std docs. See #644
124unsafe fn DenseVecStorage::remove0, unsound on bad index, but fn is unsafe. Otherwise sound.
133unsafe impl DistinctStorage for DenseVecStorage+1
143unsafe fn NullStorage::clean+1
149unsafe fn NullStorage::get+1
153unsafe fn NullStorage::get_mut+1
157unsafe fn NullStorage::insert+1
159unsafe fn NullStorage::remove+1
178unsafe impl DistinctStorage for NullStorage+1, precondition enforced by Default::default()
187unsafe fn VecStorage::clean+1
200unsafe fn VecStorage::get0, unsound on bad index, but fn is unsafe. Otherwise sound.
204unsafe fn VecStorage::get_mut0, unsound on bad index, but fn is unsafe. Otherwise sound.
208unsafe fn VecStorage::insert-1, set_len leaves uninitialized gaps in the data_id Vec, making it unsound to drop before calling clean. Sound if perfectly used (e.g. call clean), but a timebomb as stands IMO. See #644
222unsafe fn VecStorage::remove0, unsound on bad index, but fn is unsafe. Otherwise sound.
229unsafe impl DistinctStorage for VecStorage+1
EOF

src/storage/tests.rs

LineWhatNotes
460vec_arc+1, unsafe { ... }
-1, would like to see more tests involving drop types
-1, would like to see more tests involving specific storages
EOF

src/storage/track.rs

LineWhatNotes
53fn Storage::channel0, unsafe { ... } - I don't grok the safety invariants we need to hold here
59fn Storage::event_emission0, unsafe { ... } - I don't grok the safety invariants we need to hold here
72fn Storage::channel_mut0, unsafe { ... } - I don't grok the safety invariants we need to hold here
92fn Storage::set_event_emission0, unsafe { ... } - I don't grok the safety invariants we need to hold here
EOF

src/world/entity.rs

Well, this is a bit more complicated than simply doing generational indicies. Atomics... oh boy.

LineWhatNotes
51struct Allocator0, interplay of fields is underdocumented
Allocator::aliveLagging bitset derived from aliveness of generations, used for Join and not much else. Lags behind when *_atomic is used.
Allocator::raised, killedTracks atomic ops specifically for later merge, otherwise ignored/internal.
Allocator::cacheLIFO queue of entity IDs to reuse
Allocator::max_idnext entity ID if no entity IDs to reuse
90fn Allocator::kill_atomic0, leaves generations alone? Is this an async kill?
114fn Allocator::is_alive-1, doesn't this need to check !self.killed.contains(id) too?
131fn Allocator::entity-1, doesn't this need to check !self.killed.contains(id) too?
188fn Allocator::merge0, why not push directly into cache?
211struct CreateIterAtomic+1
223struct Entity+1
256struct EntitiesRes+1
321unsafe fn EntitiesRes::open+1? don't fully grok preconds but this seems fine/safe
321unsafe fn EntitiesRes::get+1? don't fully grok preconds but this seems fine/safe
336unsafe impl ParJoin for EntitiesRes-1, don't fully grok preconds
341struct EntityResBuilder+1
376struct Generation+1, should maybe have a struct-level comment that negative = dead though? Although obvious from fn is_alive...
420struct ZeroableGeneration+1
441fn ZeroableGeneration::die0, is debug_assert! enough? Seems like it'll go super off the rails in release builds, and other fns like raised panic. Internal method though...
451fn ZeroableGeneration::raised0, explicit checked_sub vs implicit for Generation::raised
470struct EntityCache0, needs better docs. Seems to be a semi-concurrent FILO index list for entity realloc.
EOF

src/world/world_ext.rs

LineWhatNotes
240fn WorldExt::create_entity_unchecked-1, does this need to be an unsafe fn? What isn't checked? Just that you have exclusive world access?
281fn WorldExt::is_alive0, sketchy edge cases
392fn World::is_alive0, should this really be panicing on a dead generation? Doesn't that just mean the entity is dead?!?
EOF

src/bitset.rs

LineWhatNotes
24unsafe fn BitSet*::open+1, sure?
29unsafe fn BitSet*::get+1, sure?
35unsafe impl ParJoin for BitSet*0, think this is fine?
EOF

src/changeset.rs

LineWhatNotes
61fn ChangeSet::add+1, unsafe { ... } - documents and abides by safety constraints
66fn ChangeSet::add+1, unsafe { ... } - abides by safety constraints, docs seem a little off
77fn ChangeSet::clear+1, unsafe { ... } - documents and abides by safety constraints
115unsafe fn &mut ChangeSet::open+1, sure?
123unsafe fn &mut ChangeSet::get-1, bypasses lifetime checks!
134unsafe fn &ChangeSet::open+1, sure?
141unsafe fn &ChangeSet::get+1, sure?
154unsafe fn ChangeSet::open+1, sure?
161unsafe fn ChangeSet::get+1, sure?
EOF

TIL

  • std::num::NonZeroI32
  • std::fmt::Formatter .debug_tuple().field().finish()
  • #[must_use] makes a lot of sense for builder patterns terminating in .build()
positive
2019-08-14
MaulingMonkey
medium, medium

Solid crate overall.

Concerns:

  • Absurdly high default NUM_RETRIES means this crate can hang.
  • Slightly unnecessarily large unsafe use, used once without clear need.
  • TempDir seems like a big footgun.
  • Well documented footguns with regards to share tmp dirs on some systems.
  • Hazard to reproducable builds thanks to random filename generation.

Detail

FileRatingNotes
src/file/imp/mod.rs+1
src/file/imp/other.rs+1not_supported
src/file/imp/unix.rs+1unsafe, but sound.
src/file/imp/windows.rs+1unsafe, but sound.
src/file/mod.rs+1
src/dir.rs+1
src/error.rs+1
src/lib.rs+1
src/spooled.rs+1Could use a .into_file()
src/util.rs0unsafe, but sound.
tests/namedtempfile.rs+1
tests/spooled.rs+1
tests/tempdir.rs+1
tests/tempfile.rs+1
.gitignore+1
Cargo.toml+1
Cargo.toml.orig+1
LICENSE-APACHE+1
LICENSE-MIT+1
NEWS+1
README.​md+1
OtherRatingNotes
unsafe0Minor unnecessary/overlong unsafe blocks
fs0Rationale of this entire crate
io+1All looks sane
docs+1Tons of doc comments
tests+1Lots of 'em

src/file/imp/unix.rs

LineWhatNotes
15cvt_err+1, verified error handling is correct vs online man pages for rename and link.
25cvt_err+1
30cstr+1
35create_named+1
44create_unlinked+1
62create+1, sane flag use.
79create+1
83create_unix+1, minor hazard to reproducable builds due to random filenames
93reopen+1
107persist0, unsafe larger than necessary, but sound.
130persist0, redox NYI but sane placeholder error
135keep0, nothing to do on unix

src/file/imp/windows.rs

LineWhatNotes
19to_utf16+1, null terminates
23create_named+1
32create+1, minor hazard to reproducable builds due to random filenames
50reopen+1, unsafe but sound. Verified error handling vs MSDN.
67keep+1, unsafe but sound. Verified error handling vs MSDN.
78persist0, unsafe larger than necessary, but sound. Verified error handling vs MSDN.

src/file/mod.rs

LineWhatNotes
...*+1, well reviewed despite my lack of notes.
587new_in+1, doc comment links wrong method (new_in instead of new)
...*+1, well reviewed despite my lack of notes.
859into_file0, confusing how to use these correctly as Drop still occurs
867into_temp_path0, confusing how to use these correctly as Drop still occurs
876into_parts0, confusing how to use these correctly as Drop still occurs
...*+1, well reviewed despite my lack of notes.

src/lib.rs

LineWhatNotes
...*+1, well reviewed despite my lack of notes.
131NUM_RETRIES-1, absurdly large default value 1 << 31, will hang "forever".
...*+1, well reviewed despite my lack of notes.

src/util.rs

LineWhatNotes
9tmpname0, unsafe for semi-pointless str::from_utf8_unchecked, but sound. Reproducable builds hazard.
26create_helper+1, although I'd pick a different error message.

TIL

let tmp;
if cond {
    tmp = asdf;
    &tmp
}
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
    f.debug_struct("TempDir")
        .field("path", &self.path())
        .finish()
}
positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2021-08-24
MaulingMonkey
low, high

[Full Audit] Terminal I/O Settings

Pros:

  • Portable as heck
  • Probably sound

Cons:

  • Limited tests
  • No verification against native C headers

Audit

⚠️ Didn't verify many OS-specific structs/constants, and termios structure might not guarantee ABI stability?

positive
2020-09-07
MaulingMonkey
medium, medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2020-09-07
MaulingMonkey
medium, high
advisories:
medium

Simple basic thread pool

Pros:

  • Documented/tested MSRV
  • Uses basic well tested mpsc channels
  • No unsafe
  • Well tested, documented, looks solid

Cons:

  • No high performance work stealing queues to break up sync points for work submission
  • No auto-scaling options
  • Ignores OS thread pools (which might have better auto-scaling magic)
  • Unclamped use of frequently mad num_cpus crate could result in ThreadPool::default causing far too many threads resulting in OOM crashes.

Full Audit

positive
2019-09-02
MaulingMonkey
low, medium

Do not use on User Generated Content!
* Vulnerable to path traversal attacks if fed bogus .tmx files (see Tileset::new_reference)
* No obvious protection against zipbombs
* A couple cases where bad input will panic, a potential DoS vector.

For game engines, there's also no great way to inject your own virtual filesystem callbacks (again see Tileset::new_reference)

There's also a few missing features:
* Wang Tiles
* Terrains
* "file" Custom Properties

Detail

FileRatingNotes
assets/tiled_base64_external.tmx+1
assets/tiled_base64_gzip.tmx+1
assets/tiled_base64_zlib.tmx+1
assets/tiled_base64.tmx+1
assets/tiled_csv.tmx+1
assets/tiled_image_layers.tmx+1
assets/tiled_xml.tmx+1
tilesheet.png+1Neat looking modern tileset... wonder what the source is!
tilesheet.tsx+1
examples/main.rs+1
src/lib.rs+1
tests/lib.rs+1
.cargo-ok+1
.gitignore+1
.travis.yml+1Sparse... no MSRV, beta/nightly, etc.
Cargo.toml+1
Cargo.toml.orig+1
CONTRIBUTORS.md+1
README.md+1Dead link to tileset source
OtherRatingNotes
unsafe+1No unsafe
fs-1See Tileset::new_reference notes
io0Brittle XML parsing, but OK for limited inputs.
docs-1Barely any.
tests+1

src/lib.rs

LineWhatNotes
28get_attrs!Eep
53parse_tag!Mishandles nested tags... fortunately that's probably not necessary.
97Colour::from_strBritish... and a possible source of panics.
161PropertyValue::newNo "file" support (see https://doc.mapeditor.org/en/stable/manual/custom-properties/#adding-properties)
238Map::newMy kingdom for some variable names!
256Map::newStill using try!
385Tileset::new_referencePossible path traversal attacks, limits ability to inject your own virtual filesystem.
860decode_csvI've heard decoding arbitrary CSV is way more complicated than this... but this probably works for tile data as used in tmx files.
883convert_to_u32Kinda want this to be based on iterators to avoid an extra alloc...
negative
2019-09-02
MaulingMonkey
medium, medium

Do not use on User Generated Content!
* Vulnerable to path traversal attacks if fed bogus .tmx files (see Tileset::new_reference)
* No obvious protection against zipbombs
* A couple cases where bad input will panic, a potential DoS vector.

For game engines, there's also no great way to inject your own virtual filesystem callbacks (again see Tileset::new_reference)

There's also a few missing features:
* Wang Tiles
* Terrains
* "file" Custom Properties

Detail

FileRatingNotes
assets/tiled_base64_external.tmx+1
assets/tiled_base64_gzip.tmx+1
assets/tiled_base64_zlib.tmx+1
assets/tiled_base64.tmx+1
assets/tiled_csv.tmx+1
assets/tiled_image_layers.tmx+1
assets/tiled_xml.tmx+1
tilesheet.png+1Neat looking modern tileset... wonder what the source is!
tilesheet.tsx+1
examples/main.rs+1
src/lib.rs+1
tests/lib.rs+1
.cargo-ok+1
.gitignore+1
.travis.yml+1Sparse... no MSRV, beta/nightly, etc.
Cargo.toml+1
Cargo.toml.orig+1
CONTRIBUTORS.md+1
README.md+1Dead link to tileset source
OtherRatingNotes
unsafe+1No unsafe
fs-1See Tileset::new_reference notes
io0Brittle XML parsing, but OK for limited inputs.
docs-1Barely any.
tests+1

src/lib.rs

LineWhatNotes
28get_attrs!Eep
53parse_tag!Mishandles nested tags... fortunately that's probably not necessary.
97Colour::from_strBritish... and a possible source of panics.
161PropertyValue::newNo "file" support (see https://doc.mapeditor.org/en/stable/manual/custom-properties/#adding-properties)
238Map::newMy kingdom for some variable names!
256Map::newStill using try!
385Tileset::new_referencePossible path traversal attacks, limits ability to inject your own virtual filesystem.
860decode_csvI've heard decoding arbitrary CSV is way more complicated than this... but this probably works for tile data as used in tmx files.
883convert_to_u32Kinda want this to be based on iterators to avoid an extra alloc...
negative
2019-09-02
MaulingMonkey
medium, medium

Do not use on User Generated Content!

  • Vulnerable to path traversal attacks if fed bogus .tmx files (see Tileset::new_reference)
  • A couple cases where bad input will panic, a potential DoS vector.

For game engines, there's also no great way to inject your own virtual filesystem callbacks (again see Tileset::new_reference)

Pros:

  • JSON is lighter weight than XML
  • Fuller format support vs tiled

Cons:

  • No compression
  • API is just as raw in many ways
  • Requires exporting.
  • Slightly unusual license for rust projects (MPL, instead of MIT/Apache 2)

Detail

FileRatingNotes
src/layer.rs0Raw structures
src/lib.rs+1
src/map.rs+1
src/object.rs+1
src/parsers.rs0No decompression support, can panic (not suitable for user generated content)
src/tile_set.rs-1Not suitable for user generated content!
src/utils.rs+1
src/wangs.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
Cargo.toml+1
Cargo.toml.orig+1
LICENSE+1
OtherRatingNotes
unsafe+1None
fs-1Path traversal
io+1serde
docs+1
tests-1Not in crate, maybe in repository

src/layer.rs

LineWhatNotes
17TileLayer::chunksOption seems kinda pointless, also this API is meh
129DrawOrderThere are at least 4 draw modes now for layers - although there's also Map / RenderOrder.... blehrg (top->down left->right, top->down right->left, ...)

src/parsers.rs

LineWhatNotes
129parse_color blueDespite earlier padding, no guarantee this is valid / may panic (both for overflowing and for not being a unicode boundary.)

src/tile_set.rs

LineWhatNotes
121Deserialize for TileSetFile::open - path traversal attacks, lack of virtual filesystem support, etc.
negative
2019-09-05
MaulingMonkey
medium, medium
issues:
high
Path traversal attacks?
issues:
medium
Panic on bad data (DoS source via UGC?)

Do not use on User Generated Content!

  • Vulnerable to path traversal attacks if fed bogus .tmx files (see Tileset::new_reference)
  • A couple cases where bad input will panic, a potential DoS vector.

For game engines, there's also no great way to inject your own virtual filesystem callbacks (again see Tileset::new_reference)

Pros:

  • JSON is lighter weight than XML
  • Fuller format support vs tiled

Cons:

  • No compression
  • API is just as raw in many ways
  • Requires exporting.
  • Slightly unusual license for rust projects (MPL, instead of MIT/Apache 2)

Detail

FileRatingNotes
src/layer.rs0Raw structures
src/lib.rs+1
src/map.rs+1
src/object.rs+1
src/parsers.rs0No decompression support, can panic (not suitable for user generated content)
src/tile_set.rs-1Not suitable for user generated content!
src/utils.rs+1
src/wangs.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
Cargo.toml+1
Cargo.toml.orig+1
LICENSE+1
OtherRatingNotes
unsafe+1None
fs-1Path traversal
io+1serde
docs+1
tests0Not in crate, but found in repository (https://gitlab.com/ljcode/tiled-json-rs/tree/master/tests)

src/layer.rs

LineWhatNotes
17TileLayer::chunksOption seems kinda pointless, also this API is meh
129DrawOrderThere are at least 4 draw modes now for layers - although there's also Map / RenderOrder.... blehrg (top->down left->right, top->down right->left, ...)

src/parsers.rs

LineWhatNotes
129parse_color blueDespite earlier padding, no guarantee this is valid / may panic (both for overflowing and for not being a unicode boundary.)

src/tile_set.rs

LineWhatNotes
121Deserialize for TileSetFile::open - path traversal attacks, lack of virtual filesystem support, etc.
positive
2019-07-24
MaulingMonkey
low, low

I'm unfamiliar with vcpkg s, but looks reasonable + all safe code.

positive
2020-08-30
MaulingMonkey
low, medium

Decent crate seeing some usage. Worth building upon. (Full Audit)

positive
2020-08-30
MaulingMonkey
low, medium

Decent crate seeing some usage. Worth building upon. (Full Audit)

positive
2020-08-30
MaulingMonkey
low, medium

Decent crate seeing some usage. Worth building upon. (Full Audit)

positive
2020-08-30
MaulingMonkey
low, medium

Decent crate seeing some usage. Worth building upon. (Full Audit)

positive
2020-08-30
MaulingMonkey
low, medium

Decent crate seeing some usage. Worth building upon. (Full Audit)

positive
2020-08-30
MaulingMonkey
low, medium

Decent crate seeing some usage. Worth building upon. (Full Audit)

negative
2020-08-30
MaulingMonkey
low, medium

vfs 0.1.0 lacks a license. Upgrade to 0.1.1+. (Full Audit)

positive
2019-07-27
MaulingMonkey
low, medium

Haven't thoroughly verified base64 logic but looks good skimming everything.

positive
2019-08-30
MaulingMonkey
medium, medium

Trivial uninhabited type. LGTM.

No unsafe, no I/O, single lib.rs source file, decent docs.

positive
2020-09-06
MaulingMonkey
high, high

Basic 0-dependencies Fn-based Waker source.

This could eventually be made safe when Wake (not Waker!) stabilizes.
In the meantime, this crate manually creates a Waker via RawWaker.
This requires some unsafe code, but this crate appropriately uses just about
the bare minimum necessary to accomplish the task, and appears to do so soundly and correctly.
Additionally, the code is minimal (63 LOC including comments and whitespace for 1.1.0) and straightforward.

Full Audit

positive
2020-09-06
MaulingMonkey
high, high

Basic 0-dependencies Fn-based Waker source.

This could eventually be made safe when Wake (not Waker!) stabilizes.
In the meantime, this crate manually creates a Waker via RawWaker.
This requires some unsafe code, but this crate appropriately uses just about
the bare minimum necessary to accomplish the task, and appears to do so soundly and correctly.
Additionally, the code is minimal (63 LOC including comments and whitespace for 1.1.0) and straightforward.

Full Audit

positive
2019-09-03
MaulingMonkey
low, low

0.13.0: RON support, lockfile/CI changes. LGTM.
0.12.0: Looks good to me. Some of the finer points are a little obtuse to me (RE: reloads, dependencies, an the inspect trait.)

Detail

FileRatingNotes
examples/toml/hello.html+1
examples/toml/main.rs+1
src/context.rs+1
src/json.rs+1
src/key.rs+1
src/lib.rs+1
src/load.rs+1
src/res.rs+1
src/toml.rs+1
tests/lib.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+1
Cargo.toml+1
Cargo.toml.orig+1
CHANGELOG.md+1
LICENSE+1
README.md+1
rustfmt.toml+1Ew 2 space indents gross
OtherRatingNotes
unsafe+1No unsafe code
fs+1Nothing fishy
io+1Nothing fishy
docs+1Good god there are a lot. Needs more concrete motivating examples though.
tests+1

src/key.rs

LineWhatNotes
12Key'static lifetime... minor leak? but I probably don't care?

TIL

cargo-sync-readme
cargo-outdated

positive
2019-08-28
MaulingMonkey
low, low

Looks good to me. Some of the finer points are a little obtuse to me (RE: reloads, dependencies, an the inspect trait.)

Detail

FileRatingNotes
examples/toml/hello.html+1
examples/toml/main.rs+1
src/context.rs+1
src/json.rs+1
src/key.rs+1
src/lib.rs+1
src/load.rs+1
src/res.rs+1
src/toml.rs+1
tests/lib.rs+1
.cargo_vcs_info.json+1
.cargo-ok+1
.gitignore+1
.travis.yml+1
Cargo.toml+1
Cargo.toml.orig+1
CHANGELOG.md+1
LICENSE+1
README.md+1
rustfmt.toml+1Ew 2 space indents gross
OtherRatingNotes
unsafe+1No unsafe code
fs+1Nothing fishy
io+1Nothing fishy
docs+1Good god there are a lot. Needs more concrete motivating examples though.
tests+1

src/key.rs

LineWhatNotes
12Key'static lifetime... minor leak? but I probably don't care?

TIL

cargo-sync-readme
cargo-outdated

positive
2019-07-27
MaulingMonkey
low, medium

Looks great overall.
I haven't double-checked any logic against WASM specs.
I haven't verified that WASM validation is suitable to rely upon for JIT compilers or the like.

Detail

FileRatingNotes
examples/dump.rs+1io (safe)
examples/simple.rs+1io (safe)
fuzz/*UNREVIEWED (excluded from crate)
src/lib.rs+1
src/limits.rs+1
src/parser.rs+1
src/tests.rs+1io (safe)
src/validator.rs+1check_utf8 could be mostly replaced with stdlib?
tests/*.wasmUnreviewed... nothing but WASM though, should be OK
.gitignore+1
.travis.yml+1
Cargo.toml+1
Cargo.toml.orig+1
check-rustfmt.sh+1Globally overwrites rustfmt with specific version
format-all.sh+1
LICENSE+1Apache 2.0
Readme.md+1
test-all.sh+1
OtherRatingNotes
unsafe+1No unsafe code.
fs+1Examples and tests only, reasonably used.
positive
2019-07-28
MaulingMonkey
low, medium

Looks great overall.
I haven't double-checked any logic against WASM specs.
I haven't verified that WASM validation is suitable to rely upon for JIT compilers or the like.

Detail

FileRatingNotes
examples/dump.rs+1io (safe)
examples/simple.rs+1io (safe)
fuzz/*UNREVIEWED (excluded from crate)
src/lib.rs+1
src/limits.rs+1
src/parser.rs+1
src/tests.rs+1io (safe)
src/validator.rs+1check_utf8 could be mostly replaced with stdlib?
tests/*.wasmUnreviewed... nothing but WASM though, should be OK
.gitignore+1
.travis.yml+1
Cargo.toml+1
Cargo.toml.orig+1
check-rustfmt.sh+1Globally overwrites rustfmt with specific version
format-all.sh+1
LICENSE+1Apache 2.0
Readme.md+1
test-all.sh+1
OtherRatingNotes
unsafe+1No unsafe code.
fs+1Examples and tests only, reasonably used.
positive
2020-08-29
MaulingMonkey
low, medium
advisories:
medium
major

Pros:

  • Excellent!
  • Easy build.rs integration

Cons:

  • Awkward to use outside of a build.rs context.

See Full Audit

positive
2019-09-03
MaulingMonkey
medium, medium

Fairly full review. Looks solid.

  • My eyes glazed over a bit when going through the decode state machine.
  • Some of the namespace stuff too.
  • Caught netvl/xml-rs#179 at least

Pros:

  • Safe code!

Cons:

  • Probably slower than quick-xml
  • Encoding XML not 100% bug free yet
  • No DTD support (yet?)
FileRatingNotes
src/reader/parser/inside_cdata.rs+1Going through tokenizer at all unnerves me slightly
src/reader/parser/inside_closing_tag_name.rs+1
src/reader/parser/inside_comment.rs+1
src/reader/parser/inside_declaration.rs+1
src/reader/parser/inside_doctype.rs+1
src/reader/parser/inside_opening_tag.rs+1
src/reader/parser/inside_processing_instruction.rs+1
src/reader/parser/inside_reference.rs+1
src/reader/parser/mod.rs+1
src/reader/parser/outside_tag.rs+1
src/reader/config.rs+1
src/reader/error.rs+1
src/reader/events.rs+1
src/reader/lexer.rs+1
src/reader/mod.rs+1
src/writer/config.rs+1Two space indents by default is heresy but whatever.
src/writer/emitter.rs0Encodings not escaped, namespace URIs not escaped. General attributes are escaped though. CDATA containing ]]> not fixed.
src/writer/events.rs+1
src/writer/mod.rs+1
src/analyze.rs+1Should really be moved to bins or examples or something.
src/attribute.rs+1
src/common.rs0Caught netvl/xml-rs#179
src/escape.rs+1
src/lib.rs+1
src/macros.rs+1
src/name.rs+1
src/namespace.rs+1
src/util.rs+1
tests/documents/sample_1_full.txt+1skimmed
tests/documents/sample_1_short.txt+1skimmed
tests/documents/sample_1.xml+1skimmed
tests/documents/sample_2_full.txt+1skimmed
tests/documents/sample_2_short.txt+1skimmed
tests/documents/sample_2.xml+1skimmed
tests/documents/sample_3_full.txt+1skimmed
tests/documents/sample_3_short.txt+1skimmed
tests/documents/sample_3.xml+1skimmed
tests/documents/sample_4_full.txt+1skimmed
tests/documents/sample_4_short.txt+1skimmed
tests/documents/sample_4.xml+1skimmed
tests/documents/sample_5_short.txt+1skimmed
tests/documents/sample_5.xml+1skimmed
tests/event_reader.rs+1
tests/event_writer.rs+1
tests/streaming.rs+1
.cargo-ok+1
.gitignore+1
.travis.yml+1Installs pip travis-cargo
Cargo.toml+1
Cargo.toml.orig+1
Changelog.md+1
design.md+1TODO list
LICENSE+1MIT, matching Cargo.toml
Readme.md+1MIT Licensed
OtherRatingNotes
unsafe+1One small use in test case, PR to remove upstream and apply deny(unsafe_code) lint.
fs+1Only in analyze (and maybe tests?), and sanely
io+1
docs+1
tests0Needs more fuzz tests

src/reader/parser/inside_reference.rs

LineWhatNotes
23predefined XML entitiesApparently these 5 are the only predefined entities in XML. Don't have to worry about the hundreds HTML supports.
52custom XML entitiesNot recursive, no XML bomb here unless DTD constructed a huge entry for extra_entities already.

TIL

  • &impl ?Sized+AsRef<str>
positive
2019-09-03
MaulingMonkey
low, medium

0.5.3: Replaced libflate with flate2, minor touchups. LGTM.
0.5.2: Looks like a solid crate. A few minor concerns:

  • 755 permissions. Necessary, but bandied about.
  • Unsanitized path names are accessible, easy to misuse.
  • Doesn't ban access to CON or similar.
  • Lacks fuzz tests

Detail

FileRatingNotes
benches/read_entry.rs+1
examples/extract_lorem.rs+1
examples/extract.rs+1
examples/file_info.rs+1
examples/stdin_info.rs+1
examples/write_dir.rs0755 permissions make me slightly nervous, but I think it's safe
examples/write_sample.rs0755 permissions make me slightly nervous, but I think it's safe
script/doc-upload.cfg+1
src/compression.rs+1
src/cp437.rs+1
src/crc32.rs+1
src/lib.rs+1
src/read.rs+1
src/result.rs+1
src/spec.rs+1
src/types.rs0Could be a little more defensive towards misue, but pretty solid.
src/write.rs+1
tests/data/*.zipUnreviewed... probably OK though
tests/end_to_end.rs+1
tests/invalid_date.rs+1
tests/zip64_large.rs+1
.gitignore+1
.travis.yml-1
appveyor.yml-1
Cargo.toml+1
Cargo.toml.orig+1
LICENSE+1MIT
README.md+1
OtherRatingNotes
unsafe+1No unsafe code
fs+1Examples/tests appear safe.
io+1
docs+1
tests+1Could use more fuzzing tests

src/types.rs

LineNotes
215I'd like this to have a scarier name... but eh, at least it's sound.
250This drops invalid components... I think it should return an error on invalid components. But at least it's sound and shouldn't be vulnerable to path navigation attacks?
250This doesn't forbid CON or similar.
298Excellent test, this is exactly what I want to see!
positive
2019-07-31
MaulingMonkey
low, medium

Looks like a solid crate. A few minor concerns:

  • 755 permissions. Necessary, but bandied about.
  • Unsanitized path names are accessible, easy to misuse.
  • Doesn't ban access to CON or similar.
  • Lacks fuzz tests

Detail

FileRatingNotes
benches/read_entry.rs+1
examples/extract_lorem.rs+1
examples/extract.rs+1
examples/file_info.rs+1
examples/stdin_info.rs+1
examples/write_dir.rs0755 permissions make me slightly nervous, but I think it's safe
examples/write_sample.rs0755 permissions make me slightly nervous, but I think it's safe
script/doc-upload.cfg+1
src/compression.rs+1
src/cp437.rs+1
src/crc32.rs+1
src/lib.rs+1
src/read.rs+1
src/result.rs+1
src/spec.rs+1
src/types.rs0Could be a little more defensive towards misue, but pretty solid.
src/write.rs+1
tests/data/*.zipUnreviewed... probably OK though
tests/end_to_end.rs+1
tests/invalid_date.rs+1
tests/zip64_large.rs+1
.gitignore+1
.travis.yml-1
appveyor.yml-1
Cargo.toml+1
Cargo.toml.orig+1
LICENSE+1MIT
README.md+1
OtherRatingNotes
unsafe+1No unsafe code
fs+1Examples/tests appear safe.
io+1
docs+1
tests+1Could use more fuzzing tests

src/types.rs

LineNotes
215I'd like this to have a scarier name... but eh, at least it's sound.
250This drops invalid components... I think it should return an error on invalid components. But at least it's sound and shouldn't be vulnerable to path navigation attacks?
250This doesn't forbid CON or similar.
298Excellent test, this is exactly what I want to see!

© bestia.dev 2021, MIT Licence, Version: 2021.1208.1729

Open source repository for this web app: https://github.com/bestia-dev/cargo_crev_web/