Skip to content

Fix more segfaults in range function #1690

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.

Sign up for GitHub

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

Jump to bottom
Closed
tpunt wants to merge 3 commits into from
Closed

Fix more segfaults in range function #1690

tpunt wants to merge 3 commits into from

Conversation

@tpunt
Copy link
Contributor

@tpunt tpunt commented Dec 22, 2015

The segfaults are caused by precision loss of large longs being converted to doubles when the step parameter is a double. The for loops continue infinitely since the step being added/subtracted upon each iteration is too small to be represented accurately as a double.

The segfaults can be reproduced with:

var_dump(count(range(PHP_INT_MIN, PHP_INT_MIN + 513, .01)));
var_dump(count(range(PHP_INT_MIN + 513, PHP_INT_MIN, .01)));

This fixes Bug #71197

@tpunt tpunt force-pushed the fix-more-segfaults-in-range-function branch from 0841683 to ed49a2a Compare December 22, 2015 16:05
@tpunt tpunt force-pushed the fix-more-segfaults-in-range-function branch from ed49a2a to b9abcd4 Compare December 22, 2015 16:23
@weltling
Copy link
Contributor

weltling commented Dec 23, 2015

@tpunt, good catch! So you think there's really no chance to fix the pre-condition? As adding one more condition to the loop which can have many iterations doesn't look nice. I haven't look deep, but there should be a way to have a formula like for highest/lowest value to double check, just for the case.

Thanks.

@tpunt
Copy link
Contributor Author

tpunt commented Dec 23, 2015

I've refactored the patch so that the range of values are calculated from the array size calculation (rather than checking the intermediary value calculated upon each iteration). This seems to be the cleanest way to do this, and has the added benefit of removing the need for the DOUBLE_DRIFT_FIX constant.

Thanks,
Tom

@jpauli jpauli added the Bug label Dec 24, 2015
@jpauli jpauli mentioned this pull request Dec 24, 2015
Closed
@jpauli
Copy link
Member

jpauli commented Dec 24, 2015

Dups with #1677 ?

@tpunt
Copy link
Contributor Author

tpunt commented Dec 24, 2015

@jpauli These two segfaults occur in the area of code that handle doubles. #1677 segfaults occur in the area of code that handles longs.

@jpauli
Copy link
Member

jpauli commented Dec 24, 2015

Ok thx for details

weltling
weltling reviewed Dec 24, 2015
View reviewed changes
ext/standard/array.c
Copy link
Contributor

@weltling weltling Dec 24, 2015

Choose a reason for hiding this comment

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

Ok, so __calc_size dosen't need to go out of scope anymore.

Copy link
Contributor Author

@tpunt tpunt Dec 24, 2015

Choose a reason for hiding this comment

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

Correct. I'll rectify that now (as well as in PR #1677).

@weltling
Copy link
Contributor

weltling commented Dec 24, 2015

@tpunt can we merge them please? It is the same function, so it'll be easier for having an overview.

Thanks.

@weltling
Copy link
Contributor

weltling commented Dec 24, 2015

I mean merge two PRs into one :)

Thanks.

@tpunt
Copy link
Contributor Author

tpunt commented Dec 24, 2015

Ok, I'll merge them now and submit a new PR.

@tpunt tpunt mentioned this pull request Dec 24, 2015
Closed
@tpunt
Copy link
Contributor Author

tpunt commented Dec 24, 2015

Superseded by PR #1695.

@tpunt tpunt closed this Dec 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tpunt @weltling @jpauli