diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ebe0100d7..bcac80ed73c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. * When the GIL is held, the refcount is now decreased immediately on drop. (Previously would wait until just before releasing the GIL.) * When the GIL is not held, the refcount is now decreased when the GIL is next acquired. (Previously would wait until next time the GIL was released.) * `FromPyObject` for `Py` now works for a wider range of `T`, in particular for `T: PyClass`. [#880](https://github.com/PyO3/pyo3/pull/880) +* Some functions such as `PyList::get_item` which return borrowed objects at the C ffi layer now return owned objects at the PyO3 layer, for safety reasons. [#890](https://github.com/PyO3/pyo3/pull/890) ### Added * `_PyDict_NewPresized`. [#849](https://github.com/PyO3/pyo3/pull/849) diff --git a/src/types/dict.rs b/src/types/dict.rs index caee04eca77..497390e786f 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -13,6 +13,7 @@ use crate::{ffi, IntoPy}; use crate::{FromPyObject, PyTryFrom}; use crate::{ToBorrowedObject, ToPyObject}; use std::collections::{BTreeMap, HashMap}; +use std::ptr::NonNull; use std::{cmp, collections, hash}; /// Represents a Python `dict`. @@ -104,8 +105,12 @@ impl PyDict { K: ToBorrowedObject, { key.with_borrowed_ptr(self.py(), |key| unsafe { - self.py() - .from_borrowed_ptr_or_opt(ffi::PyDict_GetItem(self.as_ptr(), key)) + let ptr = ffi::PyDict_GetItem(self.as_ptr(), key); + NonNull::new(ptr).map(|p| { + // PyDict_GetItem return s borrowed ptr, must make it owned for safety (see #890). + ffi::Py_INCREF(p.as_ptr()); + self.py().from_owned_ptr(p.as_ptr()) + }) }) } @@ -193,7 +198,10 @@ impl<'py> Iterator for PyDictIterator<'py> { let mut value: *mut ffi::PyObject = std::ptr::null_mut(); if ffi::PyDict_Next(self.dict.as_ptr(), &mut self.pos, &mut key, &mut value) != 0 { let py = self.dict.py(); - Some((py.from_borrowed_ptr(key), py.from_borrowed_ptr(value))) + // PyDict_Next returns borrowed values; for safety must make them owned (see #890) + ffi::Py_INCREF(key); + ffi::Py_INCREF(value); + Some((py.from_owned_ptr(key), py.from_owned_ptr(value))) } else { None } diff --git a/src/types/list.rs b/src/types/list.rs index 1ecfa1ff8ff..3eb6f09890b 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -55,9 +55,13 @@ impl PyList { /// /// Panics if the index is out of range. pub fn get_item(&self, index: isize) -> &PyAny { + assert!((index.abs() as usize) < self.len()); unsafe { - self.py() - .from_borrowed_ptr(ffi::PyList_GetItem(self.as_ptr(), index as Py_ssize_t)) + let ptr = ffi::PyList_GetItem(self.as_ptr(), index as Py_ssize_t); + + // PyList_GetItem return borrowed ptr; must make owned for safety (see #890). + ffi::Py_INCREF(ptr); + self.py().from_owned_ptr(ptr) } } diff --git a/src/types/module.rs b/src/types/module.rs index 77532d9801f..f2a3b3a74f9 100644 --- a/src/types/module.rs +++ b/src/types/module.rs @@ -72,8 +72,10 @@ impl PyModule { /// this object is the same as the `__dict__` attribute of the module object. pub fn dict(&self) -> &PyDict { unsafe { - self.py() - .from_borrowed_ptr::(ffi::PyModule_GetDict(self.as_ptr())) + // PyModule_GetDict returns borrowed ptr; must make owned for safety (see #890). + let ptr = ffi::PyModule_GetDict(self.as_ptr()); + ffi::Py_INCREF(ptr); + self.py().from_owned_ptr(ptr) } } diff --git a/src/types/set.rs b/src/types/set.rs index 40f8dadd3be..658025b92ef 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -136,7 +136,9 @@ impl<'py> Iterator for PySetIterator<'py> { let mut key: *mut ffi::PyObject = std::ptr::null_mut(); let mut hash: ffi::Py_hash_t = 0; if ffi::_PySet_NextEntry(self.set.as_ptr(), &mut self.pos, &mut key, &mut hash) != 0 { - Some(self.set.py().from_borrowed_ptr(key)) + // _PySet_NextEntry returns borrowed object; for safety must make owned (see #890) + ffi::Py_INCREF(key); + Some(self.set.py().from_owned_ptr(key)) } else { None } diff --git a/src/types/tuple.rs b/src/types/tuple.rs index 3466b39a056..f2bd6b98b68 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -70,8 +70,6 @@ impl PyTuple { /// /// Panics if the index is out of range. pub fn get_item(&self, index: usize) -> &PyAny { - // TODO: reconsider whether we should panic - // It's quite inconsistent that this method takes `Python` when `len()` does not. assert!(index < self.len()); unsafe { self.py() @@ -83,7 +81,6 @@ impl PyTuple { pub fn as_slice(&self) -> &[PyObject] { // This is safe because PyObject has the same memory layout as *mut ffi::PyObject, // and because tuples are immutable. - // (We don't even need a Python token, thanks to immutability) unsafe { let ptr = self.as_ptr() as *mut ffi::PyTupleObject; let slice = slice::from_raw_parts((*ptr).ob_item.as_ptr(), self.len());