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

Prevent Array(foo, n) from returning an array of the wrong size #8302

Closed
wants to merge 2 commits into from
Closed

Prevent Array(foo, n) from returning an array of the wrong size #8302

wants to merge 2 commits into from

Conversation

danluu
Copy link
Contributor

@danluu danluu commented Sep 11, 2014

julia> Array(Int, -14195763867060736356299439484696781493)
4611694853239872843-element Array{Int64,1}:
...

julia> x = int128(2)^65+10
36893488147419103242

julia> Array(Int, x)
10-element Array{Int64,1}:
...

The first statement only fails on my mac (10.9.2), but the second returns a mis-sized array on both linux and mac.

This could also be fixed by passing an Int128 to jl_alloc_array* and changing _new_array_. But, currently, both of my machines use uint64_t for the internal calculations because of this typedef:

#if defined(_P64) && defined(UINT128MAX)
typedef __uint128_t wideint_t;
#else
typedef uint64_t wideint_t;
#endif

Sorry for the all of the weird corner case issue spam; I spent a few minutes writing a really simplistic fuzzer. It's found a couple of bugs I could imagine actually running into, and a bunch of obscure issues like this one. IME, it's more effective to fix these really unlikely bugs than it is to try to target "good" bugs, and I've heard that mozilla had the same experience with jsfunfuzz.

@pao
Copy link
Member

pao commented Sep 11, 2014

I spent a few minutes a really simplistic fuzzer

Awesome. I know I identified a few bugs early on by "fuzzing" the REPL via fatfingering inputs.

@sjkelly
Copy link
Contributor

sjkelly commented Sep 11, 2014

For people like myself that are working to use Julia in industry, issues (with fixes!) like this are really great to see. Thanks @danluu !

@rfourquet
Copy link
Member

I think you want || instead of && in the checks?

@ivarne
Copy link
Sponsor Member

ivarne commented Sep 11, 2014

All errors worth making, is also worth fixing.

I could not help but noticing that your code is broken for abs(typemin(Int128)) > typemax(Int) 😄

The issue here is that ccall implicitly calls convert(T, x) for parameters where the type is specified as T, and it was forgotten that convert throws InexactError for BigInt, but not for Int128 (and Int64 on 32 bit platforms).

@danluu
Copy link
Contributor Author

danluu commented Sep 11, 2014

Argh! typemin(Int128) == -typemin(Int128) strikes again. Added a fix for that covers that case. Thanks @ivarne and @rfourquet .

@rfourquet
Copy link
Member

I would personally prefer not add more repetive code to Julia and have a function like check_array_dims(d::Integer...).
Also, does someone know why the exact same versions of Array constructors for 1,2 or 3 dimensions are written for both Int and Integer? efficiency?

@JeffBezanson
Copy link
Sponsor Member

I think #5413 is a better way to fix this. That will handle all such cases.

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.

6 participants