Skip to content

gh-96821: Fix undefined behaviour in _testcapimodule.c#96915

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
Fidget-Spinner merged 6 commits into from
Sep 19, 2022
Merged

gh-96821: Fix undefined behaviour in _testcapimodule.c#96915

Fidget-Spinner merged 6 commits into from
Sep 19, 2022

Conversation

@matthiasgoergens
Copy link
Contributor

@matthiasgoergensmatthiasgoergens commented Sep 18, 2022•
edited
Loading

The first commit demonstrates the undefined behaviour and is meant to fail tests.

The other commits fix it.

@matthiasgoergensmatthiasgoergens mentioned this pull request Sep 18, 2022
Closed
@matthiasgoergensmatthiasgoergens changed the title gh-96821: Assert for demonstrating undefined behaviourgh-96821: Fix undefined behaviour in _testcapimodule.cSep 18, 2022
@matthiasgoergens
Copy link
ContributorAuthor

matthiasgoergens commented Sep 18, 2022

@kumaraditya303@mdickinson Would you please have a look?

@CAM-GerlachCAM-Gerlach added the extension-modules C modules in the Modules dir label Sep 18, 2022
matthiasgoergensand others added 2 commits September 19, 2022 07:42
@matthiasgoergens@CAM-Gerlach
Update Misc/NEWS.d/next/Core and Builtins/2022-09-18-08-47-40.gh-issu…
1810e7b
…e-96821.Co2iOq.rst Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@matthiasgoergens
Merge branch 'main' into testcapi_ub
4f06f53
Fidget-Spinner
Fidget-Spinner approved these changes Sep 19, 2022
View reviewed changes
Copy link
Member

@Fidget-SpinnerFidget-Spinner left a comment

Choose a reason for hiding this comment

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

I think I've wrote similar code before elsewhere (just doing raw pointer arithmetic on an unverified args pointer). However, I don't recall where exactly now :(.

Anyways, thanks this LGTM.

@matthiasgoergens
Copy link
ContributorAuthor

matthiasgoergens commented Sep 19, 2022

Thanks!

@Fidget-SpinnerFidget-Spinner merged commit cbdeda8 into python:mainSep 19, 2022
@Fidget-SpinnerFidget-Spinner added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 19, 2022
@miss-islington
Copy link
Contributor

miss-islington commented Sep 19, 2022

Thanks @matthiasgoergens for the PR, and @Fidget-Spinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Sep 19, 2022

Thanks @matthiasgoergens for the PR, and @Fidget-Spinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

bedevere-bot commented Sep 19, 2022

GH-96926 is a backport of this pull request to the 3.10 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.10 only security fixes label Sep 19, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 19, 2022
@matthiasgoergens@miss-islington
pythongh-96821: Fix undefined behaviour in_testcapimodule.c(pytho…
a25c419
…nGH-96915) * pythongh-96821: Assert for demonstrating undefined behaviour * Fix UB Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> (cherry picked from commit cbdeda8) Co-authored-by: Matthias Görgens <matthias.goergens@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 19, 2022
@matthiasgoergens@miss-islington
pythongh-96821: Fix undefined behaviour in_testcapimodule.c(pytho…
3eb6a15
…nGH-96915) * pythongh-96821: Assert for demonstrating undefined behaviour * Fix UB Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> (cherry picked from commit cbdeda8) Co-authored-by: Matthias Görgens <matthias.goergens@gmail.com>
@bedevere-botbedevere-bot removed the needs backport to 3.11 only security fixes label Sep 19, 2022
@bedevere-bot
Copy link

bedevere-bot commented Sep 19, 2022

GH-96927 is a backport of this pull request to the 3.11 branch.

@matthiasgoergens
Copy link
ContributorAuthor

matthiasgoergens commented Sep 19, 2022

I think I've wrote similar code before elsewhere (just doing raw pointer arithmetic on an unverified args pointer). However, I don't recall where exactly now :(.

There was an example of this in eg ceval.c. Not sure if that one was yours?

Btw, this kind of null pointer arithmetic is perfectly defined, as long as we compile with -fwrapv. I'm just working on cleaning things up so we can get rid of that flag and enjoy a few more C compiler optimisations.

Fidget-Spinner pushed a commit that referenced this pull request Sep 19, 2022
@miss-islington@CAM-Gerlach@matthiasgoergens
gh-96821: Fix undefined behaviour in_testcapimodule.c(GH-96915) (G…
88a3f18
…H-96926) * gh-96821: Assert for demonstrating undefined behaviour * Fix UB (cherry picked from commit cbdeda8) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Matthias Görgens <matthias.goergens@gmail.com>
Fidget-Spinner pushed a commit that referenced this pull request Sep 19, 2022
@miss-islington@CAM-Gerlach@matthiasgoergens
gh-96821: Fix undefined behaviour in_testcapimodule.c(GH-96915) (G…
3bf8e0f
…H-96927) * gh-96821: Assert for demonstrating undefined behaviour * Fix UB (cherry picked from commit cbdeda8) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Matthias Görgens <matthias.goergens@gmail.com>
pablogsal pushed a commit that referenced this pull request Oct 24, 2022
@miss-islington@CAM-Gerlach
@matthiasgoergens@pablogsal
gh-96821: Fix undefined behaviour in_testcapimodule.c(GH-96915) (G…
9e008fe
…H-96927) * gh-96821: Assert for demonstrating undefined behaviour * Fix UB (cherry picked from commit cbdeda8) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM> Co-authored-by: Matthias Görgens <matthias.goergens@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CAM-GerlachCAM-GerlachCAM-Gerlach left review comments

@Fidget-SpinnerFidget-SpinnerFidget-Spinner approved these changes

Assignees

No one assigned

Labels

extension-modulesC modules in the Modules dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@matthiasgoergens@miss-islington@bedevere-bot@CAM-Gerlach@Fidget-Spinner