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

Bump ordered-float to v1.1.1 in v0.0.x due to potential UB #292

Closed
wants to merge 1 commit into from

Conversation

ClementTsang
Copy link

As per RUSTSEC-2020-0082, version 1.1.1 of the ordered-float dependency fixes a potential cause of UB.

This doesn't appear to affect the master branch, but it does affect v0.0.x versions.

@ClementTsang
Copy link
Author

CI fails seem to be related to the version of rust (1.36) not having support for some features, and not due to the change?

@ClementTsang ClementTsang changed the title Bump ordered-float to v1.1.1 due to potential UB Bump ordered-float to v1.1.1 in v0.0.x due to potential UB Dec 8, 2020
@svartalf
Copy link
Member

svartalf commented Dec 9, 2020

Yeah, I think that either MSRV for v0.0.x branch is set improperly or one of the dependencies just bumped MSRV causing this issue; it needs to be investigated additionally.

Do you have any plans to switch to heim = "0.1" eventually? Current beta is pretty much stable already and next one beta will be promoted to stable release soon.
Upgrading will effectively solve your issue here :)

@svartalf
Copy link
Member

svartalf commented Dec 9, 2020

In addition: I just checked and heim is not doing any calculations with NotNan (this one file is the only place where it is used), so I seriously doubt we can hit UB here.

@jamtur01
Copy link

jamtur01 commented Dec 9, 2020

We're on v0.1.0-beta.3 and still seeing the issue.

error[A001]: ordered_float:NotNan may contain NaN after panic in assignment operators
    ┌─ /home/james/src/vector/Cargo.lock:346:1
    │
346 │ ordered-float 1.0.2 registry+https://github.com/rust-lang/crates.io-index
    │ ------------------------------------------------------------------------- security vulnerability detected
    │
    = ID: RUSTSEC-2020-0082
    = Advisory: https://rustsec.org/advisories/RUSTSEC-2020-0082
    = After using an assignment operators such as `NotNan::add_assign`, `NotNan::mul_assign`, etc., it was possible for the resulting `NotNan` value to contain a `NaN`.  This could cause undefined behavior in safe code, because the safe `NotNan::cmp` method contains internal unsafe code that assumes the value is never `NaN`.  (It could also cause undefined behavior in third-party unsafe code that makes the same assumption, as well as logic errors in safe code.)

      This was mitigated starting in version 0.4.0, by panicking if the assigned value is NaN.  However, in affected versions from 0.4.0 onward, code that uses the `NotNan` value during unwinding, or that continues after catching the panic, could still observe the invalid value and trigger undefined behavior.

      The flaw is fully corrected in versions 1.1.1 and 2.0.1, by ensuring that the assignment operators panic without modifying the operand, if the result would be `NaN`.
    = Announcement: https://github.com/reem/rust-ordered-float/pull/71
    = Solution: Upgrade to >=1.1.1, <2.0.0 OR >=2.0.1
    = ordered-float v1.0.2
      └── heim-process v0.1.1-beta.3
          └── heim v0.1.0-beta.3
              └── vector v0.12.0

@svartalf
Copy link
Member

svartalf commented Dec 9, 2020

@jamtur01 yes, ordered-float crate for v0.1.x were already updated in 8240de5 and will be published together with #289 soon, so it should'nt be a problem for you and other users of newer versions.

My main question if it is worth to backport this change to older versions, it might be easier for @ClementTsang to upgrade?

@ClementTsang
Copy link
Author

@svartalf Didn't realize the current beta was pretty much stable; if that's the case I'll probably just give upgrading a try instead.

I'll close the PR then.

@ClementTsang ClementTsang deleted the v0.0.x branch October 6, 2021 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants