From b7ecec781212081ceb9a617c9a9a00c6207008ed Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sat, 2 May 2020 23:07:46 +0100 Subject: [PATCH 1/2] Remove unsound return of borrowed objects --- CHANGELOG.md | 1 + src/types/dict.rs | 14 +++++++++++--- src/types/list.rs | 8 ++++++-- src/types/module.rs | 6 ++++-- src/types/set.rs | 4 +++- src/types/tuple.rs | 9 ++++----- 6 files changed, 29 insertions(+), 13 deletions(-) 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..cc8be95ef18 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -70,12 +70,12 @@ 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() - .from_borrowed_ptr(ffi::PyTuple_GET_ITEM(self.as_ptr(), index as Py_ssize_t)) + // PyTuple_GET_ITEM return borrowed ptr; must make owned for safety (see #890). + let ptr = ffi::PyTuple_GET_ITEM(self.as_ptr(), index as Py_ssize_t); + ffi::Py_INCREF(ptr); + self.py().from_owned_ptr(ptr) } } @@ -83,7 +83,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()); From 6f74fe6b38f9c4addfc67bbdac2c05a6ce42e169 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sun, 3 May 2020 13:39:19 +0100 Subject: [PATCH 2/2] Allow borrowed object for PyTuple::get_item. As per feedback on #890 --- src/types/tuple.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/types/tuple.rs b/src/types/tuple.rs index cc8be95ef18..f2bd6b98b68 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -72,10 +72,8 @@ impl PyTuple { pub fn get_item(&self, index: usize) -> &PyAny { assert!(index < self.len()); unsafe { - // PyTuple_GET_ITEM return borrowed ptr; must make owned for safety (see #890). - let ptr = ffi::PyTuple_GET_ITEM(self.as_ptr(), index as Py_ssize_t); - ffi::Py_INCREF(ptr); - self.py().from_owned_ptr(ptr) + self.py() + .from_borrowed_ptr(ffi::PyTuple_GET_ITEM(self.as_ptr(), index as Py_ssize_t)) } }