Skip to content
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.

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

Avoid UB in PySlice::indices #2061

Merged
merged 2 commits into from
Dec 20, 2021
Merged

Avoid UB in PySlice::indices #2061

merged 2 commits into from
Dec 20, 2021

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Dec 20, 2021

Mutating a variables that is not declared with mut is undefined behavior.

Mutating a variables that is not declared with `mut` is undefined
behavior.
@birkenfeld
Copy link
Member

Just to check that my understanding is correct: the problem is not the missing let mut but the mutation through a &shared ref, right? And let mut is necessary to get a &mut ref.

@Amanieu
Copy link
Contributor Author

Amanieu commented Dec 20, 2021

From the list of undefined behavior in the reference:

Mutating immutable data. All data inside a const item is immutable. Moreover, all data reached through a shared reference or data owned by an immutable binding is immutable, unless that data is contained within an UnsafeCell<U>.

@davidhewitt
Copy link
Member

Thank you for submitting this, appreciate the fix! Out of curiosity, did you find this via automated tooling or because you happened to be browsing the PyO3 source?

I might take a moment to check our other pointer casts in case this pattern was also employed elsewhere.

@Amanieu
Copy link
Contributor Author

Amanieu commented Dec 20, 2021

I was working on some code which used indices and had a look into the source code to see how it dealt with the isize/usize differences. Then I noticed the bug which immediately stood out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants