Merge pull request #890 from davidhewitt/no-borrowed-objects

Remove unsound return of borrowed objects

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<T>` 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)

src/types/dict.rs

@@ -10,6 +10,7 @@ use crate::{
     ffi, AsPyPointer, FromPyObject, IntoPy, PyTryFrom, Python, ToBorrowedObject, ToPyObject,
 };
 use std::collections::{BTreeMap, HashMap};
+use std::ptr::NonNull;
 use std::{cmp, collections, hash};
 
 /// Represents a Python `dict`.
@@ -101,8 +102,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())
+            })
         })
     }
 
@@ -190,7 +195,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
             }

src/types/list.rs

@@ -54,9 +54,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)
         }
     }
 

src/types/module.rs

@@ -71,8 +71,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::<PyDict>(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)
         }
     }
 

src/types/set.rs

@@ -135,7 +135,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
             }

src/types/tuple.rs

@@ -69,8 +69,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()
@@ -82,7 +80,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());