Add review for atty v0.2.14
This commit is contained in:
parent
e1928d3224
commit
9b82ee95fb
|
@ -0,0 +1,73 @@
|
||||||
|
----- BEGIN CREV PROOF -----
|
||||||
|
kind: package review
|
||||||
|
version: -1
|
||||||
|
date: 2022-09-19T17:10:02.580323647+02:00
|
||||||
|
from:
|
||||||
|
id-type: crev
|
||||||
|
id: YdnEoYtqvbBGv0hhENLDUYmc3tNfm5V5NIG5hCovHyM
|
||||||
|
url: https://git.jcg.re/jcgruenhage/crev-proofs.git
|
||||||
|
package:
|
||||||
|
source: https://crates.io
|
||||||
|
name: atty
|
||||||
|
version: 0.2.14
|
||||||
|
revision: 7b5df17888997d57c2c1c8f91da1db5691f49953
|
||||||
|
digest: WJaqS5xu6ORo2px2Rku1ynYYN4TuCdxeS8LR68ngyw4
|
||||||
|
review:
|
||||||
|
thoroughness: high
|
||||||
|
understanding: high
|
||||||
|
rating: positive
|
||||||
|
flags:
|
||||||
|
unmaintained: true
|
||||||
|
alternatives:
|
||||||
|
- source: https://crates.io
|
||||||
|
name: is-terminal
|
||||||
|
comment: |-
|
||||||
|
One of the more foundational crates found in the dependency tree of a lot of
|
||||||
|
rust programs, because both clap and env_logger pull this in.
|
||||||
|
|
||||||
|
In my review I've fully read the source code and can confirm that I fully
|
||||||
|
understand what's happening in here. The unix and hermit targets are
|
||||||
|
extremely straight-forward. As for the windows target, that's a bit more
|
||||||
|
complicated, but still manageable in the end. Windows doesn't have a clear
|
||||||
|
API for determining whether something is a (pseudo) TTY, so the heuristics
|
||||||
|
provided by this crate are as good as it's going to get.
|
||||||
|
|
||||||
|
This crate has quite a few unsafe code sections, but that's sometimes
|
||||||
|
required for providing a safe interface. In this case, we need it because the
|
||||||
|
underlying functions for unix (libc) are unsafe, and the same applies to a
|
||||||
|
bunch of winapi functions used in the heuristics for windows.
|
||||||
|
|
||||||
|
The bits of unsafe code that's not just wrapping an unsafe function provided
|
||||||
|
by another library are all in the windows heuristics, and involved
|
||||||
|
provisioning buffers that winapi calls can write info back into and some
|
||||||
|
pointer magic. While I couldn't spot an issue with this, a look into the issue
|
||||||
|
tracker revealed that other's have. The buffer creation on the heap is not
|
||||||
|
necessarily aligned properly, meaning that there's a possible soundness issue
|
||||||
|
on windows targets here.
|
||||||
|
|
||||||
|
Last but not least, we have to talk about maintenance and and alternatives:
|
||||||
|
The soundness issue mentioned above has been known for over a year, with a fix
|
||||||
|
first pushed to a PR shortly after. Even though there's been reviews from
|
||||||
|
well-known rustaceans, the author hasn't merged this PR yet, and in general
|
||||||
|
there hasn't been any relevant activity for a while.
|
||||||
|
|
||||||
|
An alternative implementation that has been derived from `atty` is
|
||||||
|
`is-terminal`, which has taken this into consideration and has taken over the
|
||||||
|
rest of the implementation from here, with a slightly different API. They're
|
||||||
|
also switching the underlying implementations around, reducing the amount of
|
||||||
|
unsafe.
|
||||||
|
|
||||||
|
As for why I'm still rating this as positive: With the exception of the
|
||||||
|
potential unsoundness bug on windows, this crate is still okay-ish to be used.
|
||||||
|
Also: Getting rid of it soon is not realistic, because it's in the dependency
|
||||||
|
tree of quite a few widely-used crates.
|
||||||
|
|
||||||
|
To summarize: Widely used, foundational crate. Except for on windows, this is
|
||||||
|
perfectly fine, on windows there's an unsoundness bug, and there has not been
|
||||||
|
an update or other activity from upstream. `is-terminal` is a good
|
||||||
|
alternative, which was derived from this crate, but includes the fix for the
|
||||||
|
unsoundess bug.
|
||||||
|
----- SIGN CREV PROOF -----
|
||||||
|
mY2ODgsozEL4os5aFk0hxL8jH5SHu7awErNdlLHY9UhDL-vLdbDjEIYiD0wFPaTTglTxirK41Jwtokpm4rWyDg
|
||||||
|
----- END CREV PROOF -----
|
||||||
|
|
Loading…
Reference in a new issue