tailhook / humantime

A parser and formatter for std::time::{SystemTime, Duration}
Apache License 2.0
283 stars 34 forks source link

Made crate unsafe free and forbid unsafe #13

Closed gh0st42 closed 4 years ago

gh0st42 commented 4 years ago

While checking one of my own crates with cargo geiger I was surprised to see that humantime makes use of unsafe code. After a quick look at the code it looks safe at the moment but this does not mean that nothing of the code before the unsafe utf8 conversion will ever change and bugs will be introduced in the future. Also other people might just look at the output of tools like geiger or crev and think that humantime is insecure. This crate is awesome, small, fast and does what it is supposed to do, I love it :)

I replaced the unsafe code with the straight forward safe variant and benchmarked the speed penalty on my machine:

Old code using unsafe:

running 2 tests
test rfc3339_chrono            ... bench:         628 ns/iter (+/- 53)
test rfc3339_humantime_seconds ... bench:          48 ns/iter (+/- 2)

test result: ok. 0 passed; 0 failed; 0 ignored; 2 measured; 0 filtered out

     Running target/release/deps/datetime_parse-7557b3a3c6ced56e

running 6 tests
test datetime_utc_parse_millis  ... bench:         116 ns/iter (+/- 5)
test datetime_utc_parse_nanos   ... bench:         120 ns/iter (+/- 3)
test datetime_utc_parse_seconds ... bench:         104 ns/iter (+/- 2)
test rfc3339_humantime_millis   ... bench:          19 ns/iter (+/- 0)
test rfc3339_humantime_nanos    ... bench:          23 ns/iter (+/- 0)
test rfc3339_humantime_seconds  ... bench:          16 ns/iter (+/- 0)

New safe code:

running 2 tests
test rfc3339_chrono            ... bench:         644 ns/iter (+/- 22)
test rfc3339_humantime_seconds ... bench:          54 ns/iter (+/- 2)

test result: ok. 0 passed; 0 failed; 0 ignored; 2 measured; 0 filtered out

     Running target/release/deps/datetime_parse-7557b3a3c6ced56e

running 6 tests
test datetime_utc_parse_millis  ... bench:         113 ns/iter (+/- 7)
test datetime_utc_parse_nanos   ... bench:         119 ns/iter (+/- 4)
test datetime_utc_parse_seconds ... bench:         104 ns/iter (+/- 5)
test rfc3339_humantime_millis   ... bench:          19 ns/iter (+/- 0)
test rfc3339_humantime_nanos    ... bench:          23 ns/iter (+/- 0)
test rfc3339_humantime_seconds  ... bench:          16 ns/iter (+/- 0)

So the safe variant is slightly slower on my machine but not significantly. As an added bonus I added #![forbid(unsafe_code)] to the lib which lets cargo geiger output a safe lock symbol, clearly indicating that it is absolutely safe to use this crate.

I do understand if you reject this PR because your code currently is safe even though it might not be obvious to all users of this crate.

tailhook commented 4 years ago

While I'm yet not sure where the attitude of removing unsafe code everywhere will lead us in general. In this case yes, I think the overhead does not worth optimizing. Having less to worry about for users matters more.

Merged. Thanks!

fbernier commented 2 years ago

FYI, we've had to fork this code in a project where this is called in a hot loop and the 4% speed difference for us is very worth it. I know this type of change is all about tradeoffs but I personally believe this is easy enough to audit and worth optimizing for.