From dd3b41de6031a9cd9426ba71bd6f217d5e62e0dc Mon Sep 17 00:00:00 2001 From: Chris Morgan Date: Fri, 14 Jan 2022 04:18:31 +1100 Subject: [PATCH] Add review for same-file v1.0.6 --- .../reviews/2022-01-package-jqCnIw.proof.crev | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/QE5OVlHZ4QyOcMqdjXhS1MgsoZHvUqxOHNZwyfpsDIU/reviews/2022-01-package-jqCnIw.proof.crev b/QE5OVlHZ4QyOcMqdjXhS1MgsoZHvUqxOHNZwyfpsDIU/reviews/2022-01-package-jqCnIw.proof.crev index 84c715d..9a0bca2 100644 --- a/QE5OVlHZ4QyOcMqdjXhS1MgsoZHvUqxOHNZwyfpsDIU/reviews/2022-01-package-jqCnIw.proof.crev +++ b/QE5OVlHZ4QyOcMqdjXhS1MgsoZHvUqxOHNZwyfpsDIU/reviews/2022-01-package-jqCnIw.proof.crev @@ -447,3 +447,91 @@ comment: |- ex_TABmVr_CTEY6B5C-NDKk7ciKdCK1bLf6ltjpXyoVknvyAC2AnRjA5rlPX_5ORQMeHmzJ2UxWUR0RdgBjYAQ ----- END CREV PROOF ----- +----- BEGIN CREV PROOF ----- +kind: package review +version: -1 +date: "2022-01-14T02:22:52.039384447+11:00" +from: + id-type: crev + id: QE5OVlHZ4QyOcMqdjXhS1MgsoZHvUqxOHNZwyfpsDIU + url: "https://git.chrismorgan.info/crev-proofs" +package: + source: "https://crates.io" + name: same-file + version: 1.0.6 + revision: 5799cd323b8eefd17a089c950dac113f66c89c9e + digest: Yf7qyYw3W3WoHK4t74epRoJkZehfhWh-Hh-R0LfB8UQ +review: + thoroughness: high + understanding: high + rating: neutral +comment: |- + The good: + + ✓ Decently tested, including CI coverage of the declared MSRV (1.34.0). (As + noted in src/lib.rs lines 485–491, edge case tests are difficult to + synthesise; Andrew doesn’t know how to create those environmental + conditions and I don’t really either. I suppose it’d entail reading file + system specs and creating suitable file systems from scratch, and mounting + them—not terribly difficult, but tedious work for limited gain.) + + ✓ The crate only supports its *functionality* on cfg(any(unix, windows)), but + still provides a sane fallback on cfg(not(any(unix, windows))) so that all + methods returns an explanatory I/O error. + + ✓ It’s unfortunate that false positives can arise, but given that it’s + unavoidable, it is pleasingly well-document. + + ✓ Cargo.toml excludes CI files from the package (keep things small!) + + The neutral: + + • The I/O error returned on unknown platforms is of kind Other. I look + forward to an MSRV bump to 1.53.0 so that Unsupported can be used. + + • The code has some cfg(target_os = "redox") that can be removed when the + MSRV reaches Rust 1.38.0 (that’s when redox was moved to the unix family, + in ). + + • Andrew uses his preferred Unlicense/MIT combo; [I have problems with + Unlicense ]. + + • src/win.rs lines 17–32 acknowledge a straightforwardly-remediable gap that, + if resolved, would by the sound of it eliminate false positives on at least + the Windows Server + ReFS combo. This is something that someone that cared + could contribute fairly easily, with access to Windows Server 2012 or + later. (Microsoft’s documentation of FILE_ID_INFO *says* it’s Server-only, + but given that ReFS is supported from Windows 8.1 onwards as well, I wonder + if it might be *de facto* available elsewhere too.) + + • Handle::{stdin, stdout, stderr} bypass std::io::{Stdin, Stdout, Stderr} + locking. I don’t much like this, but given that avoiding it would be very + painful to the API, and that locking isn’t a memory-safety matter, already + not being inviolable due to foreign code (though the std::io docs don’t + seem to mention that), so I think this was the pragmatic choice. + + The bad: + + ✗ The file size comparison that is documented in two places as being there on + Windows as a false positive avoidance heuristic is accidentally missing. + This was reported and acknowledged as an error on 2021-05-19 in + , but it still hasn’t + been reinstated or the comments removed. + + ✗ Handle::{dev, ino} are Unix-specific methods; it would be better not to + expose them on the type itself, but to move them to a platform-specific + extension trait to make it more explicit, e.g. HandleUnixExt. + + ✗ std::os::unix::io::IntoRawFd implementation panics on Handle::{stdin, + stdout, stderr}, due to careless Option handling (and the unwraps are + accompanied by comments assuring you that it won’t panic 🙁). + ManuallyDrop would be a better choice than Option here. Demonstration: + ``std::os::unix::io::IntoRawFd::into_raw_fd(same_file::Handle::stdin().unwrap())``. + Reported at https://github.com/BurntSushi/same-file/issues/55. + + I was going to rate it positive until I found this panic (the first two bads + don’t warrant a downgrade). Fix just that, and I’ll update it to positive. +----- SIGN CREV PROOF ----- +GlyWB-6rwNvY0qOppjx4AcRh1ta-btcpZ6OBaqPUg_Nr5WGCYWiNsfNWYENFHDLZpnPViTt7YaW78oPepAHhCA +----- END CREV PROOF ----- + -- 2.42.0