BUG: Do not rely on undefined casting behavior in numpy.random.#9680
BUG: Do not rely on undefined casting behavior in numpy.random.#9680yangli2 wants to merge 9 commits intonumpy:masterfrom
Conversation
…andardized behavior. See http://en.cppreference.com/w/c/language/conversion#Real_floating-integer_conversions. This is potentially dangerous and makes the code fail with tools such as AddressSanitizer. Adding checks here to prevent overflow during casting and make sure we get the desired behavior.
Current double to long casting in the zipf function depends on non-standardized behavior. See http://en.cppreference.com/w/c/language/conversion#Real_floating-integer_conversions. This is potentially dangerous and makes the code fail with tools such as AddressSanitizer. Adding checks here to prevent overflow during casting and make sure we get the desired behavior.
Current double to long casting in the zipf function depends on non-standardized behavior. See http://en.cppreference.com/w/c/language/conversion#Real_floating-integer_conversions. This is potentially dangerous and makes the code fail with tools such as AddressSanitizer. Adding checks here to prevent overflow during casting and make sure we get the desired behavior.
Current double to long casting in the zipf function depends on non-standardized behavior. See http://en.cppreference.com/w/c/language/conversion#Real_floating-integer_conversions. This is potentially dangerous and makes the code fail with tools such as AddressSanitizer. Adding checks here to prevent overflow during casting and make sure we get the desired behavior.
numpy/random/mtrand/distributions.c
Outdated
There was a problem hiding this comment.
I'd just use X without the cast to long and only cast it to long for the return. Note that every place else it is cast float again for the computation and that it is non-negative, so the second check may be avoided. So maybe something like
if (X > LONG_MAX) {
/* X < 1 will be rejected */
X = 0.0;
continue;
}
and then put X < 1 check below first to avoid the big computation. Note that the continue above skips the division by X, so 0.0 is not a problem. Also we put the opening brace on the same line as the condition, see doc/C_STYLE_GUIDE.rst.txt.
There was a problem hiding this comment.
Thx for the review! Makes sense, done in my latest commit. We don't need to set X to 0, right? It seems that continue would cause a new X to be computed already. For X<1 we can skip both the big computation as well as the computation in the condition; so I added both conditions in the loop.
There was a problem hiding this comment.
No, continue goes to the conditional in a do loop.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, right. Fixed that.
| T = pow(1.0 + 1.0/X, am1); | ||
| } while (((V*X*(T-1.0)/(b-1.0)) > (T/b)) || X < 1); | ||
| return X; | ||
| if (X > LONG_MAX) { |
There was a problem hiding this comment.
I'd probably just do
if (X > LONG_MAX || X < 1.) {
and remove the condition for calculating T.
|
You can squash your commits together, do |
BUG: Fixes for np.random.zipf
|
Closed in #9824 |
Current double to long casting in the zipf function depends on non-standardized behavior. See http://en.cppreference.com/w/c/language/conversion#Real_floating-integer_conversions.
This is potentially dangerous and makes the code fail with tools such as AddressSanitizer.
Adding checks here to prevent overflow during casting and make sure we get the desired behavior.