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

Make assignment operators panic safe #71

Merged
merged 1 commit into from Dec 6, 2020
Merged

Conversation

branpk
Copy link
Contributor

@branpk branpk commented Dec 6, 2020

Current behavior allows UB in safe code.

@mbrubeck
Copy link
Collaborator

mbrubeck commented Dec 6, 2020

Current behavior allows UB in safe code.

Can you explain how? This should not be possible, because there is no unsafe code executed in these methods.

@mbrubeck
Copy link
Collaborator

mbrubeck commented Dec 6, 2020

Oh, I understand now. During unwinding, or if the panic is caught, a NotNan ends up containing NaN. This can lead to UB in unsafe code that assumes NotNan never contains NaN, including the implementation of NotNan::cmp which can be called from safe code.

Good catch, and thanks!

@mbrubeck mbrubeck merged commit c55cda3 into reem:master Dec 6, 2020
mbrubeck added a commit that referenced this pull request Dec 6, 2020
Release notes:

* Make assignment operators panic safe (#71). This fixes a bug that
could cause undefined behavior in safe code.
mbrubeck added a commit that referenced this pull request Dec 6, 2020
Release notes:

* Make assignment operators panic safe (#71). This fixes a bug that
could cause undefined behavior in safe code.
mbrubeck added a commit to mbrubeck/advisory-db that referenced this pull request Dec 6, 2020
…erators

After using an assignment operators such as `NotNat::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 continued after using unwinding to catch this 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`.

Fix details:

reem/rust-ordered-float#20
reem/rust-ordered-float#71
mbrubeck added a commit to mbrubeck/advisory-db that referenced this pull request Dec 6, 2020
…erators

After using an assignment operators such as `NotNat::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 continued after using unwinding to catch this 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`.

Fix details:

reem/rust-ordered-float#20
reem/rust-ordered-float#71
mbrubeck added a commit to mbrubeck/advisory-db that referenced this pull request Dec 6, 2020
…erators

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 continued after using unwinding to catch this 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`.

Fix details:

reem/rust-ordered-float#20
reem/rust-ordered-float#71
@mbrubeck
Copy link
Collaborator

mbrubeck commented Dec 6, 2020

Published ordered-float 1.1.1 and 2.0.1, and submitted a security advisory: rustsec/advisory-db#514

@mbrubeck
Copy link
Collaborator

mbrubeck commented Dec 6, 2020

I haven't fixed this on the ordered-float 0.5 branch, because backporting is non-trivial and the test suite on that branch uses unstable features that are broken in modern Rust toolchains. There are only a few actively-developed crates still using ordered-float 0.5, and the ones with source code available (descartes, prolog_parser, scryer-prolog) are not impacted by this bug.

Anyone using ordered-float 0.5 should upgrade to a newer version as soon as possible.

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

2 participants