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

proposal: encoding/json: Unmarshal overflowing numbers into math/big types #24502

Closed
jzelinskie opened this issue Mar 23, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@jzelinskie
Copy link
Contributor

commented Mar 23, 2018

When unmarshaling JSON, an overflowing number is treated as if there was an error parsing the number:

go/src/encoding/json/decode.go

Lines 1035 to 1040 in 50921bf

case reflect.Float32, reflect.Float64:
n, err := strconv.ParseFloat(s, v.Type().Bits())
if err != nil || v.OverflowFloat(n) {
d.saveError(&UnmarshalTypeError{Value: "number " + s, Type: v.Type(), Offset: int64(d.readIndex())})
break
}

Instead, an additional branch could added that attempts to unmarshal the number into a big.Int or big.Float.

I understand this could be contentious because RFC7159 recommends setting some limits to size of numbers in JSON, but strictly speaking there isn't one.


I'm personally running into this when trying to encode/decode cryptographic keys. I know that I could avoid this by declaring the type that I want to unmarshal, but I'm attempting to maintain run-time dynamic types for different types of keys.

@gopherbot gopherbot added this to the Proposal milestone Mar 23, 2018

@gopherbot gopherbot added the Proposal label Mar 23, 2018

@dsnet dsnet changed the title proposal: unmarshal overflowing numbers into math/big types proposal: encoding/json: Unmarshal overflowing numbers into math/big types Mar 23, 2018

@dsnet

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

The math/big.Int type already supports the MarshalJSON and UnmarshalJSON methods. Thus, fields of that type should already handle big numbers. Thus, in the function you are pointing to, *big.Int should be handled here:

u, ut, pv := indirect(v, isNull)
if u != nil {
err := u.UnmarshalJSON(item)
if err != nil {
return err
}
return nil
}

before it even gets to the switch you pointed to.

@dsnet

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

That being said, math/big.Float does not support MarshalJSON and UnmarshalJSON. If that's the case, perhaps those methods should be added instead? I don't believe json should gain a dependency on big.

Edit: Nevermind. json calls the UnmarshalText methods if it is present.

@jzelinskie

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

@dsnet Yup, I'm aware of that, which I acknowledge in the addendum to my proposal.
Not wanting json to depend on math/big is a fair opinion.

@dsnet

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

When you say dynamic runtime, you mean something like this?

var m map[string]interface{}
json.Unmarshal([]byte(`{"k1": "string", "k2": null, "k3": 1234, "k4": 12345678901234567890}`), &m)
fmt.Printf("%T\n", m["k3"])
fmt.Printf("%T\n", m["k4"]) // you expect to see *big.Int?
@jzelinskie

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

@dsnet Yup! I actually found a workaround for my particular use-case, but I made this issue to probe interest in enabling exactly the code you've written.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

I'm slightly against this, since JSON in practice has to interact with the least common denominator of all the JSON in the ecosystem, and JavaScript can't even handle 64-bit integers, so I can't imagine there's many JSON documents in the world with a JSON number with so many digits that it'd overflow int64 or uint64 (the latter of which Java doesn't have either).

I could be convinced if you somebody presented actual uses cases of JSON in the wild with such huge and unquoted number literals, and pointed out how other JSON parsers transparently deal with it.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

No one will expect *big.Int in the interface. It sounds like in your program you should be using json.Decoder.UseNumber.

@rsc rsc closed this Apr 2, 2018

@golang golang locked and limited conversation to collaborators Apr 2, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.