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

hex2num is vulnerable to overflow #22031

Closed
KristofferC opened this issue May 23, 2017 · 9 comments
Closed

hex2num is vulnerable to overflow #22031

KristofferC opened this issue May 23, 2017 · 9 comments
Labels
kind:bug Indicates an unexpected problem or unintended behavior needs decision A decision on this change is needed

Comments

@KristofferC
Copy link
Sponsor Member

From https://discourse.julialang.org/t/bigint-to-hex-and-back-to-bigint/3873.

The documentation simply says:

  hex2num(str)

  Convert a hexadecimal string to the floating point number it represents.

and does not hint on any limit of size in the allowed hex string.

hex2num should probably detect the overflow and try parsing the string as a BigInt before converting it?

@StefanKarpinski
Copy link
Sponsor Member

How about we delete this instead? It should be part of either parsing or reinterpret. In what sense does a hexadecimal string represent a floating-point number? cc @JeffBezanson; at the very least, the doc string should explain this.

@rfourquet
Copy link
Member

I think the difference between parse and hex2num is the same as between bin and bits for integers, i.e. hex2num is concerned only by the machine bit-representation. Cf. #14418.

@StefanKarpinski
Copy link
Sponsor Member

We have parse to convert hex strings into 64-bit chunks of data and reinterpret to convert that into a Float64, so I'm not sure why this function is fundamental enough to exist in Base.

@rfourquet rfourquet added the kind:bug Indicates an unexpected problem or unintended behavior label May 23, 2017
@simonbyrne
Copy link
Contributor

+1 for getting rid of this: I think it's a Matlab/Octave-ism that has hung around. It's also not a great pattern to encourage, as it's technically endian-dependent (if you want lossless text-based serialisation, you should be using hex float literals).

@stevengj
Copy link
Member

stevengj commented May 24, 2017

I agree that hex2num seems a bit out of place, and it lends itself to questionable usage.

The only registered package that seems to use this is CBOR.jl, which somewhat perversely converts bytes to a string (via bytes2hex) and then to floating-point data, making at least 3 copies of the data in the process (array -> slice -> string -> float), rather than converting the bytes directly to a float via reinterpret. (Similarly for num2hex in CBOR encoding.)

@simonbyrne
Copy link
Contributor

It's also type-unstable.

simonbyrne added a commit that referenced this issue May 26, 2017
simonbyrne added a commit that referenced this issue May 26, 2017
@simonbyrne
Copy link
Contributor

Also, num2hex doesn't appear to handle negative integers correctly.

julia> num2hex(-1)
"-0000000000000001"

@simonbyrne simonbyrne added the needs decision A decision on this change is needed label May 29, 2017
@rfourquet
Copy link
Member

Also, num2hex doesn't appear to handle negative integers correctly.

This is fixed in #22039 (which addresses also the hex2num type-instability if the user is willing to specify a destination type).

@simonbyrne simonbyrne mentioned this issue May 30, 2017
19 tasks
rfourquet added a commit that referenced this issue Jun 3, 2017
simonbyrne added a commit that referenced this issue Aug 10, 2017
@fredrikekre
Copy link
Member

Given #22088, is this still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug Indicates an unexpected problem or unintended behavior needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

7 participants