◐ Shell
reader mode source ↗
Skip to content

gh-96821: Fix undefined behaviour in audioop.c#96923

Merged
mdickinson merged 11 commits into
python:mainfrom
matthiasgoergens:matthias/fix-audioop-UB
Oct 10, 2022
Merged

gh-96821: Fix undefined behaviour in audioop.c#96923
mdickinson merged 11 commits into
python:mainfrom
matthiasgoergens:matthias/fix-audioop-UB

Conversation

@matthiasgoergens

@matthiasgoergens matthiasgoergens commented Sep 19, 2022 •
edited
Loading

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.

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

matthiasgoergens commented Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

This was referenced Sep 21, 2022
Merged
Open
@kumaraditya303

kumaraditya303 commented Sep 26, 2022 •
edited
Loading

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.

mdickinson
mdickinson approved these changes Oct 8, 2022
View reviewed changes

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

Hide comment

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

@bedevere-bot bedevere-bot added and removed awaiting review labels Oct 8, 2022
@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.

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

@matthiasgoergens

matthiasgoergens commented Oct 9, 2022

Copy link
Copy Markdown
Contributor Author

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

@matthiasgoergens

matthiasgoergens commented Oct 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@mdickinson @JelleZijlstra I applied the suggested changes.

Thanks for having a look!

@mdickinson mdickinson merged commit 553d3c1 into python:main 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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@matthiasgoergens @kumaraditya303 @mdickinson @JelleZijlstra @bedevere-bot