advisory-db icon indicating copy to clipboard operation
advisory-db copied to clipboard

fast-floats is unsound [nightly-only crate]

Open scottmcm opened this issue 4 years ago • 7 comments

https://lib.rs/crates/fast-floats

The crate exposes the fadd_fast (and similar) intrinsics to safe code behind the operator traits: https://docs.rs/fast-floats/0.1.2/src/fast_floats/lib.rs.html#93-101

This is unsound because using NAN as an argument to one of those intrinsics produces poison: https://llvm.org/docs/LangRef.html#fast-math-flags

Which can then be used to produce UB by using it "as an instruction operand that has any values that trigger undefined behavior": https://llvm.org/docs/LangRef.html#poisonvalues

And one can directly create a NAN FF32 in safe code: https://docs.rs/fast-floats/0.1.2/src/fast_floats/lib.rs.html#61

(As well as in other ways, like creating FF32s with 0.0 and dividing them.)

scottmcm avatar Feb 04 '21 02:02 scottmcm

Have you reported this to the fast-float developers?

alex avatar Feb 04 '21 02:02 alex

I have not. Given the description of "experimental and unstable; for experiments" I suspect they know, or at least wouldn't be too concerned about it.

I was just trying to come up with an appropriate place to discourage its accidental use outside of those contexts, given that it's sometimes recommended to people (like https://users.rust-lang.org/t/whats-the-equivalent-of-icc-fp-model-fast-2/55110/2?u=scottmcm).

scottmcm avatar Feb 04 '21 02:02 scottmcm

I believe the way forward here is to report this to the upstream issue tracker. If this is by design, and they have no plans to remove the issue anytime soon, it seems reasonable to issue an advisory.

Shnatsel avatar Feb 04 '21 02:02 Shnatsel

cc @bluss . the repo does not seem to have an issue tracker, https://github.com/bluss/fast-floats.

jorgecarleitao avatar Oct 31 '21 21:10 jorgecarleitao

Hi. It was last discussed here. https://github.com/bluss/fast-floats/pull/1#issuecomment-753508048

I.e it's a know issue, but nothing's using this crate. Fixing it would be the next step - if it had development.

I would say it's reasonable if I update it with an 0.2 version and make the constructors unsafe. That should be better for the safety situation rather than just marking the crate deprecated.

@scottmcm: The crate as published implements all assign-ops as += due to a bug, just another datapoint for how hopelessly unused it is heh.

bluss avatar Oct 31 '21 23:10 bluss

version 0.2.0 is now released. I wouldn't mind if it's marked unmaintained, but I guess let's do whatever is less fuss.

The crate exposes the fadd_fast (and similar) intrinsics to safe code behind the operator traits: https://docs.rs/fast-floats/0.1.2/src/fast_floats/lib.rs.html#93-101

This is still true, but the constructor for Fast requires unsafe

This is unsound because using NAN as an argument to one of those intrinsics produces poison: https://llvm.org/docs/LangRef.html#fast-math-flags

Which can then be used to produce UB by using it "as an instruction operand that has any values that trigger undefined behavior": https://llvm.org/docs/LangRef.html#poisonvalues

And one can directly create a NAN FF32 in safe code: https://docs.rs/fast-floats/0.1.2/src/fast_floats/lib.rs.html#61

(As well as in other ways, like creating FF32s with 0.0 and dividing them.)

Zero and Default traits are removed, so there should be no safe constructors for FF32(0.) left.

bluss avatar Nov 01 '21 17:11 bluss

@scottmcm or @bluss would you like to help and send advisory PR on this - I know this was like last year's thing but it would be educational resource to the community regardless despite the circumstances :woman_shrugging:

pinkforest avatar Sep 08 '22 11:09 pinkforest