Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fast-floats is unsound [nightly-only crate] #743

Open
scottmcm opened this issue Feb 4, 2021 · 7 comments
Open

fast-floats is unsound [nightly-only crate] #743

scottmcm opened this issue Feb 4, 2021 · 7 comments
Labels
Unsound Informational / Unsound

Comments

@scottmcm
Copy link

scottmcm commented Feb 4, 2021

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.)

@alex
Copy link
Member

alex commented Feb 4, 2021

Have you reported this to the fast-float developers?

@scottmcm
Copy link
Author

scottmcm commented Feb 4, 2021

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).

@Shnatsel
Copy link
Member

Shnatsel commented Feb 4, 2021

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.

@jorgecarleitao
Copy link
Contributor

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

@bluss
Copy link

bluss commented Oct 31, 2021

Hi. It was last discussed here. bluss/fast-floats#1 (comment)

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
Copy link

bluss commented Nov 1, 2021

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.

@pinkforest
Copy link
Contributor

@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 🤷‍♀️

@amousset amousset added the Unsound Informational / Unsound label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Unsound Informational / Unsound
Projects
None yet
Development

No branches or pull requests

7 participants