Skip to content

bpo-37798: Prevent undefined behavior in direct calls to the C helper… #16149

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
Merged
merged 1 commit into from
Sep 15, 2019
Merged

bpo-37798: Prevent undefined behavior in direct calls to the C helper… #16149

merged 1 commit into from
Sep 15, 2019

Conversation

@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Sep 14, 2019 •
edited by bedevere-bot
Loading

@rhettinger
bpo-37798: Prevent undefined behavior in direct calls to the C helper…
0ad67ed 
… function.

While the _statistics helper is private and not intended to be called
directly, it is possible to do so.  That would bypass the parameter
validation checks in the pure python calling code.

Accordingly, we need to add parameter checks to avoid undefined
behavior (division by zero) or errors in the underlying functions
(negative square root or non-positive argument for logarithm).
@rhettinger rhettinger added type-bug An unexpected behavior, bug, or error skip news needs backport to 3.8 labels Sep 14, 2019
@rhettinger rhettinger requested a review from corona10 September 14, 2019 18:19
corona10
corona10 approved these changes Sep 15, 2019
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@rhettinger LGTM for the code.
2 questions for this PR.

  1. Don't we have to add this logic for pure python version?

    cpython/Lib/statistics.py

    Line 821 in ff603f6

    def _normal_dist_inv_cdf(p, mu, sigma):

  2. We don't need unit tests for _normal_dist_inv_cdf with error handling?

@corona10
Copy link
Member

corona10 commented Sep 15, 2019

@rhettinger
When I think about it, the two cases above seem to be no problem
It's good to go :)

@rhettinger rhettinger merged commit 6e27a0d into python:master Sep 15, 2019
@rhettinger rhettinger deleted the statistics_undefined branch September 15, 2019 16:36
@bedevere-bot
Copy link

bedevere-bot commented Sep 15, 2019

@rhettinger: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

miss-islington commented Sep 15, 2019

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Sep 15, 2019

GH-16160 is a backport of this pull request to the 3.8 branch.

rhettinger added a commit that referenced this pull request Sep 15, 2019
@miss-islington @rhettinger
bpo-37798: Prevent undefined behavior in direct calls to the C helper…
d6fdfc8 
… function. (GH-16149) (GH-16160)

(cherry picked from commit 6e27a0d)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes

Assignees

No one assigned

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rhettinger @corona10 @bedevere-bot @miss-islington @the-knights-who-say-ni