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

CBOR Writer: Use canonical NaN representation for NaN values #92934

Merged
merged 5 commits into from
Oct 6, 2023

Conversation

tomeksowi
Copy link
Contributor

RFC 7049 (CBOR) specifies "If NaN is an allowed value, it must always be represented as 0xf97e00". The only exception I made is when the user explicitly requests precision (FP size) is preserved.

The problem occurred for x86, C# defines NaN as 0.0/0.0 which yields -NaN on x86 FP units, which gets encoded as 0xf9fe00.

Fixes issue #92080

Also, use positive NaNs on RISC-V for FP ops crossgen-evaluated when host is x86.

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03 @sirntar @yurai007

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 3, 2023
@ghost
Copy link

ghost commented Oct 3, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

RFC 7049 (CBOR) specifies "If NaN is an allowed value, it must always be represented as 0xf97e00". The only exception I made is when the user explicitly requests precision (FP size) is preserved.

The problem occurred for x86, C# defines NaN as 0.0/0.0 which yields -NaN on x86 FP units, which gets encoded as 0xf9fe00.

Fixes issue #92080

Also, use positive NaNs on RISC-V for FP ops crossgen-evaluated when host is x86.

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03 @sirntar @yurai007

Author: tomeksowi
Assignees: -
Labels:

area-System.Security, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Oct 3, 2023

Tagging subscribers to this area: @dotnet/area-system-formats-cbor, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

RFC 7049 (CBOR) specifies "If NaN is an allowed value, it must always be represented as 0xf97e00". The only exception I made is when the user explicitly requests precision (FP size) is preserved.

The problem occurred for x86, C# defines NaN as 0.0/0.0 which yields -NaN on x86 FP units, which gets encoded as 0xf9fe00.

Fixes issue #92080

Also, use positive NaNs on RISC-V for FP ops crossgen-evaluated when host is x86.

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03 @sirntar @yurai007

Author: tomeksowi
Assignees: -
Labels:

area-System.Formats.Cbor, community-contribution

Milestone: -

@tomeksowi tomeksowi force-pushed the cbor-writer-nan branch 3 times, most recently from 0df3b49 to f704df9 Compare October 3, 2023 15:23
@tomeksowi tomeksowi marked this pull request as ready for review October 4, 2023 12:27
@tomeksowi tomeksowi requested a review from vcsjones October 4, 2023 12:27
@krwq krwq requested a review from tannergooding October 5, 2023 08:26
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JIT change LGTM.

@tomeksowi tomeksowi requested a review from krwq October 5, 2023 10:35
@eiriktsarpalis eiriktsarpalis self-assigned this Oct 5, 2023
RFC 7049 (CBOR) specifies "If NaN is an allowed value, it must always be represented as 0xf97e00". The only exception is when the user explicitly requests precision (FP size) is preserved.

The problem occurred for x86, C# defines NaN as 0.0/0.0 which yields -NaN on x86 FP units, which gets encoded as 0xf9fe00.

Fixes issue dotnet#92080
NaNs only get written as 4 or 8 bytes only in CTAP2 mode, which requires to preserve all bits anyway.

+ review fixes
@eiriktsarpalis eiriktsarpalis merged commit e146bed into dotnet:main Oct 6, 2023
106 of 109 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 9, 2023
steveharter pushed a commit to steveharter/runtime that referenced this pull request Jan 10, 2024
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.

The PR fixes that by extending the lock statement.

Fix dotnet#92934
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Formats.Cbor community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants