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 `audioop.c` by matthiasgoergens · Pull Request #96923 · python/cpython

Conversation

@matthiasgoergens

@matthiasgoergens matthiasgoergens commented Sep 19, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Left-shifting negative numbers is undefined behaviour.

Fortunately, multiplication works just as well, is (implementation) defined behaviour, and gets compiled to the same machine code as before by optimizing compilers.

  • 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: Fix undefined behaviour in audioop.c
13b5850 
Left-shifting negative numbers is undefined behaviour.

Fortunately, multiplication works just as well, is defined behaviour,
and gets compiled to the same machine code as before by optimizing
compilers.
@bedevere-bot bedevere-bot added the awaiting review label Sep 19, 2022
@blurb-it
📜🤖 Added by blurb_it.
07d709a
@matthiasgoergens

matthiasgoergens commented Sep 19, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@kumaraditya303 @mdickinson @erlend-aasland Would you please have a look? Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@matthiasgoergens
Merge branch 'main' into matthias/fix-audioop-UB
fd6f822
This was referenced Sep 21, 2022
gh-96821: Add config option --with-strict-overflow #96823
Merged
Enable -fstrict-overflow #96821
Open
@matthiasgoergens
Merge branch 'main' into matthias/fix-audioop-UB
a3502da
@kumaraditya303

kumaraditya303 commented Sep 26, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Sorry but I won't have time for this in the near future, Mark or Erlend can take a look.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@matthiasgoergens
Merge branch 'main' into matthias/fix-audioop-UB
98b2352
JelleZijlstra
JelleZijlstra reviewed Oct 4, 2022
View reviewed changes
Comment thread Modules/audioop.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
matthiasgoergens added 2 commits October 5, 2022 09:18
@matthiasgoergens
Merge remote-tracking branch 'origin/main' into matthias/fix-audioop-UB
c549094
@matthiasgoergens
Move asserts for more coverage
15e43a6
mdickinson
mdickinson approved these changes Oct 8, 2022
View reviewed changes

@mdickinson mdickinson 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.

The changes LGTM at least in principle; I haven't done much in the way of testing.

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 Oct 8, 2022
@matthiasgoergens
Merge branch 'main' into matthias/fix-audioop-UB
4e36000
mdickinson
mdickinson reviewed Oct 9, 2022
View reviewed changes
Comment thread Modules/audioop.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
mdickinson
mdickinson reviewed Oct 9, 2022
View reviewed changes
Comment thread Modules/audioop.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
mdickinson
mdickinson reviewed Oct 9, 2022
View reviewed changes
Comment thread Modules/audioop.c Resolved

Uh oh!

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

mdickinson commented Oct 9, 2022

Copy link
Copy Markdown
Member

@matthiasgoergens Can we revert the changes at lines 173 and 262 of the original code? I'm fairly sure that those changes aren't necessary, because the quantity being shifted is always nonnegative.
All reactions

Sorry, something went wrong.

Uh oh!

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

mdickinson commented Oct 9, 2022

Copy link
Copy Markdown
Member

Can we revert the changes at lines 173 and 262 of the original code?

To be clear, I'm happy with the rest of the changes as-is, so if @JelleZijlstra is also happy, I think we can merge after those two reversions.
All reactions

Sorry, something went wrong.

Uh oh!

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

matthiasgoergens commented Oct 9, 2022

Copy link
Copy Markdown
Contributor Author

@mdickinson Thanks. I'll revert the lines you asked about.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
matthiasgoergens added 3 commits October 9, 2022 18:30
@matthiasgoergens
Merge branch 'main' into matthias/fix-audioop-UB
c3f9108
@matthiasgoergens
Merge remote-tracking branch 'origin/main' into matthias/fix-audioop-UB
da7aab3
@matthiasgoergens
Revert unneeded changes
ba80611
@matthiasgoergens

matthiasgoergens commented Oct 9, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@mdickinson @JelleZijlstra I applied the suggested changes.

Thanks for having a look!
mdickinson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra approved these changes Oct 9, 2022
View reviewed changes
@mdickinson
mdickinson merged commit 553d3c1 into python:main Oct 10, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 10, 2022
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@matthiasgoergens @blurb-it @mpage
pythongh-96821: Fix undefined behaviour in audioop.c (python#96923)
6d91946 
* pythongh-96821: Fix undefined behaviour in `audioop.c`

Left-shifting negative numbers is undefined behaviour.

Fortunately, multiplication works just as well, is defined behaviour,
and gets compiled to the same machine code as before by optimizing
compilers.

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
+1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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 @kumaraditya303 @mdickinson @JelleZijlstra @bedevere-bot
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 01402520 — 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.