Skip to content

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

Merged
Fidget-Spinner merged 6 commits into
python:mainfrom
matthiasgoergens:testcapi_ub
Sep 19, 2022
Merged

gh-96821: Fix undefined behaviour in _testcapimodule.c#96915
Fidget-Spinner merged 6 commits into
python:mainfrom
matthiasgoergens:testcapi_ub

Conversation

@matthiasgoergens

@matthiasgoergensmatthiasgoergens commented Sep 18, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

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

matthiasgoergens commented Sep 18, 2022

Copy link
Copy Markdown
ContributorAuthor

@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
CAM-Gerlach
CAM-Gerlach reviewed Sep 18, 2022
View reviewed changes
Comment threadMisc/NEWS.d/next/Core and Builtins/2022-09-18-08-47-40.gh-issue-96821.Co2iOq.rst Outdated
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

@Fidget-SpinnerFidget-Spinner left a comment

Copy link
Copy Markdown
Member

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

matthiasgoergens commented Sep 19, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks!

@Fidget-Spinner
Fidget-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

miss-islington commented Sep 19, 2022

Copy link
Copy Markdown
Contributor

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

@miss-islington

miss-islington commented Sep 19, 2022

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

bedevere-bot commented Sep 19, 2022

Copy link
Copy Markdown

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

bedevere-bot commented Sep 19, 2022

Copy link
Copy Markdown

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

@matthiasgoergens

matthiasgoergens commented Sep 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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