GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-96821: Fix undefined behaviour in `_testcapimodule.c` by matthiasgoergens · Pull Request #96915 · python/cpython

Conversation

@matthiasgoergens

@matthiasgoergens matthiasgoergens commented Sep 18, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

The other commits fix it.

  • Issue: Enable -fstrict-overflow #96821

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@matthiasgoergens
pythongh-96821: Assert for demonstrating undefined behaviour
cd374dd
@bedevere-bot bedevere-bot added the awaiting review label Sep 18, 2022
@matthiasgoergens
Fix UB
5f88886
@matthiasgoergens matthiasgoergens mentioned this pull request Sep 18, 2022
PEP 7: Mention signed overflow explicitly python/peps#2796
Closed
@matthiasgoergens matthiasgoergens changed the title gh-96821: Assert for demonstrating undefined behaviour gh-96821: Fix undefined behaviour in _testcapimodule.c Sep 18, 2022
matthiasgoergens and others added 2 commits September 18, 2022 16:46
@matthiasgoergens
Merge remote-tracking branch 'origin/main' into testcapi_ub
97f6aea
@blurb-it
📜🤖 Added by blurb_it.
01d7490
@matthiasgoergens

matthiasgoergens commented Sep 18, 2022

Copy link
Copy Markdown
Contributor Author

@kumaraditya303 @mdickinson Would you please have a look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CAM-Gerlach CAM-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 thread Misc/NEWS.d/next/Core and Builtins/2022-09-18-08-47-40.gh-issue-96821.Co2iOq.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
matthiasgoergens and 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 <[email protected]>
@matthiasgoergens
Merge branch 'main' into testcapi_ub
4f06f53
Fidget-Spinner
Fidget-Spinner approved these changes Sep 19, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 19, 2022
@matthiasgoergens

matthiasgoergens commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner
Fidget-Spinner merged commit cbdeda8 into python:main Sep 19, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 19, 2022
@Fidget-Spinner Fidget-Spinner added needs backport to 3.10 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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.10 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 <[email protected]>
(cherry picked from commit cbdeda8)

Co-authored-by: Matthias Görgens <[email protected]>
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 <[email protected]>
(cherry picked from commit cbdeda8)

Co-authored-by: Matthias Görgens <[email protected]>
@bedevere-bot bedevere-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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@matthiasgoergens

matthiasgoergens commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
Co-authored-by: Matthias Görgens <[email protected]>
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 <[email protected]>
Co-authored-by: Matthias Görgens <[email protected]>
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 <[email protected]>
Co-authored-by: Matthias Görgens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

extension-modules C modules in the Modules dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@matthiasgoergens @miss-islington @bedevere-bot @CAM-Gerlach @Fidget-Spinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.