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

Jump to bottom

expm1 may incorrectly raise OverflowError on underflow #90176

Closed
zooba opened this issue Dec 9, 2021 · 11 comments
Closed

expm1 may incorrectly raise OverflowError on underflow #90176

zooba opened this issue Dec 9, 2021 · 11 comments
Assignees
@zooba
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir

Comments

@zooba
Copy link
Member

zooba commented Dec 9, 2021

BPO 46018
Nosy @mdickinson, @zooba, @miss-islington, @miss-islington, @miss-islington
PRs
  • bpo-46018: Ensure that math.expm1 does not raise on underflow #29997
  • [3.10] bpo-46018: Ensure that math.expm1 does not raise on underflow (GH-29997) #30012
  • [3.10] bpo-46018: Ensure that math.expm1 does not raise on underflow (GH-29997) #30012
  • [3.10] bpo-46018: Ensure that math.expm1 does not raise on underflow (GH-29997) #30012
  • [3.9] bpo-46018: Ensure that math.expm1 does not raise on underflow (GH-29997) #30013
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/zooba'
closed_at = <Date 2021-12-09.19:16:56.094>
created_at = <Date 2021-12-09.00:43:56.135>
labels = ['library', '3.9', '3.10', '3.11']
title = 'expm1 may incorrectly raise OverflowError on underflow'
updated_at = <Date 2021-12-09.19:37:18.067>
user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2021-12-09.19:37:18.067>
actor = 'miss-islington'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2021-12-09.19:16:56.094>
closer = 'steve.dower'
components = ['Library (Lib)']
creation = <Date 2021-12-09.00:43:56.135>
creator = 'steve.dower'
dependencies = []
files = []
hgrepos = []
issue_num = 46018
keywords = ['patch']
message_count = 11.0
messages = ['408059', '408082', '408083', '408137', '408139', '408145', '408148', '408151', '408152', '408154', '408157']
nosy_count = 3.0
nosy_names = ['mark.dickinson', 'steve.dower', 'miss-islington', 'miss-islington', 'miss-islington']
pr_nums = ['29997', '30012', '30012', '30012', '30013']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue46018'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']
    @zooba
    Copy link
    Member Author

    zooba commented Dec 9, 2021

    If a C runtime's math functions set errno to ERANGE, we assume it is a valid underflow if fabs(result) < 1.0.

    However, because expm1 includes a -1.0, it underflows towards -1.0. This fails the above check, and so if a runtime's expm1() sets ERANGE we will raise a spurious OverflowError.

    @zooba zooba added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Dec 9, 2021
    @zooba zooba self-assigned this Dec 9, 2021
    @zooba zooba added stdlib Python modules in the Lib dir 3.9 only security fixes labels Dec 9, 2021
    @mdickinson
    Copy link
    Member

    It's a bit cheap and nasty, but I think we could simply replace the line:

    if (fabs(x) < 1.0)

    in is_error with

    if (fabs(x) < 2.0)

    perhaps with an explanatory comment. All we need to do is distinguish underflow from overflow, and 2.0 is still clearly a _long_ way away from any overflow boundary.

    It would be good to have a test that would trigger the behaviour, too.

    @mdickinson
    Copy link
    Member

    I presume this is also worth an upstream report? Setting ERANGE on a result that's close to -1.0 is rather questionable.

    @zooba
    Copy link
    Member Author

    zooba commented Dec 9, 2021

    I considered just switching to <2.0, but wasn't sure if I would be breaking some other unspoken behaviour there. But you're right, it's really just detecting underflow vs. overflow, so that's a much simpler way to check.

    I've filed the upstream report. I suspect the errno is coming from the exp() call within the expm1() implementation, so it's underflowing towards 0.0 and then subtracting 1.

    @zooba
    Copy link
    Member Author

    zooba commented Dec 9, 2021

    I've also got no idea how to write a test for this, given that it's a very thin wrapper around a platform's C runtime library. Our existing test discovered when the library changed behaviour to start setting errno, which is probably the best we can do.

    @mdickinson
    Copy link
    Member

    I've also got no idea how to write a test for this

    Yep, that's fine. All I want is that at least one particular value that caused the spurious OverflowError is in the test suite somewhere, but it sounds as though that's already the case. I'd imagine that one of these two testcases should be enough to trigger it:

    cpython/Lib/test/math_testcases.txt

    Lines 495 to 496 in 44b0e76

    expm10200 expm1 -38.0 -> -1.0
    expm10210 expm1 -710.0 -> -1.0

    @zooba
    Copy link
    Member Author

    zooba commented Dec 9, 2021

    Lines 500-504 are the ones that trigger it. Apparently there are no tests in that file for straight exp(), but the equivalent tests for those would return 0.0 and suppress ERANGE too.

    @mdickinson
    Copy link
    Member

    Lines 500-504 are the ones that trigger it.

    Ah, right. Thanks.

    Apparently there are no tests in that file for straight exp()

    Yes - that file was mostly written to give good coverage for places where we'd written our own implementations rather than simply wrapping an existing libm function, though I think we've now reverted to using the libm expm1 in all cases.

    @zooba
    Copy link
    Member Author

    zooba commented Dec 9, 2021

    New changeset 3363e1c by Steve Dower in branch 'main':
    bpo-46018: Ensure that math.expm1 does not raise on underflow (GH-29997)
    3363e1c

    @miss-islington
    Copy link
    Contributor

    New changeset 5ae4265 by Miss Islington (bot) in branch '3.9':
    bpo-46018: Ensure that math.expm1 does not raise on underflow (GH-29997)
    5ae4265

    @zooba zooba closed this as completed Dec 9, 2021
    @miss-islington
    Copy link
    Contributor

    New changeset ca08655 by Miss Islington (bot) in branch '3.10':
    bpo-46018: Ensure that math.expm1 does not raise on underflow (GH-29997)
    ca08655

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @zooba zooba

    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @mdickinson @zooba @miss-islington