Skip to content

Commit 9e8bf42

Browse files
Close a reference leak in smart pointers for managing PyObject pointers.
Fix some erroneous noexcept statements. Also add some more helpful assert statements.
1 parent fc522f4 commit 9e8bf42

File tree

1 file changed

+20
-16
lines changed

1 file changed

+20
-16
lines changed

dynd/include/utility_functions.hpp

+20-16
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ class py_ref_t {
258258
*/
259259
explicit py_ref_t(PyObject *obj, bool consume_ref) noexcept
260260
{
261+
PYDYND_ASSERT_IF(not_null, obj != nullptr);
261262
o = obj;
262263
if (consume_ref) {
263264
decref_if_owned<!owns_ref, not_null>(o);
@@ -278,13 +279,6 @@ class py_ref_t {
278279
decref_if_owned<owns_ref, false>(o);
279280
}
280281

281-
// For assignment operators, only allow assignment
282-
// in cases where implicit conversions are also allowed.
283-
// This forces explicit handling of cases that could
284-
// need to have an exception raised.
285-
// For that reason, these are all marked as noexcept.
286-
// Assignment never comsumes a reference.
287-
288282
/* If:
289283
* This type allows null or the input type does not,
290284
* Then:
@@ -314,7 +308,7 @@ class py_ref_t {
314308
*/
315309
template <bool other_owns_ref, bool other_not_null,
316310
typename std::enable_if_t<!other_not_null && not_null> * = nullptr>
317-
py_ref_t<owns_ref, not_null> &operator=(const py_ref_t<other_owns_ref, other_not_null> &other) noexcept
311+
py_ref_t<owns_ref, not_null> &operator=(const py_ref_t<other_owns_ref, other_not_null> &other)
318312
{
319313
if (other.o != nullptr) {
320314
// Assert that the input reference is valid.
@@ -332,6 +326,8 @@ class py_ref_t {
332326
}
333327

334328
// Same as previous two, except these assign from rvalues rather than lvalues.
329+
// Only allow move assignment if the input type owns its reference.
330+
// Otherwise, the copy assignment operator is sufficient.
335331

336332
/* If:
337333
* this type allows null,
@@ -341,9 +337,8 @@ class py_ref_t {
341337
* Then:
342338
* Assignment from the other py_ref_t type to this type may not raise an exception.
343339
*/
344-
template <bool other_owns_ref, bool other_not_null,
345-
typename std::enable_if_t<other_not_null || !not_null> * = nullptr>
346-
py_ref_t<owns_ref, not_null> &operator=(py_ref_t<other_owns_ref, other_not_null> &&other) noexcept
340+
template <bool other_not_null, typename std::enable_if_t<other_not_null || !not_null> * = nullptr>
341+
py_ref_t<owns_ref, not_null> &operator=(py_ref_t<true, other_not_null> &&other) noexcept
347342
{
348343
// If the input reference should not be null, assert that that is the case.
349344
PYDYND_ASSERT_IF(other_not_null, other.o != nullptr);
@@ -354,7 +349,12 @@ class py_ref_t {
354349
decref_if_owned<owns_ref, false>(o);
355350
o = other.o;
356351
other.o = nullptr;
357-
incref_if_owned<owns_ref && !other_owns_ref, not_null>(o);
352+
// If type being assigned to does not hold its own reference,
353+
// Decref the input reference.
354+
decref_if_owned<!owns_ref, false>(o);
355+
// Assert that, after possibly decrefing the input reference,
356+
// that the stored reference is still valid.
357+
PYDYND_ASSERT_IF(o != nullptr, Py_REFCNT(o) > 0);
358358
return *this;
359359
}
360360

@@ -364,9 +364,8 @@ class py_ref_t {
364364
* Then:
365365
* Assignment from the other py_ref_t type to this type may raise an exception.
366366
*/
367-
template <bool other_owns_ref, bool other_not_null,
368-
typename std::enable_if_t<!other_not_null && not_null> * = nullptr>
369-
py_ref_t<owns_ref, not_null> &operator=(py_ref_t<other_owns_ref, other_not_null> &&other) noexcept
367+
template <bool other_not_null, typename std::enable_if_t<!other_not_null && not_null> * = nullptr>
368+
py_ref_t<owns_ref, not_null> &operator=(py_ref_t<true, other_not_null> &&other)
370369
{
371370
if (other.o != nullptr) {
372371
// Assert that the input reference is valid.
@@ -376,7 +375,12 @@ class py_ref_t {
376375
decref_if_owned<owns_ref, false>(o);
377376
o = other.o;
378377
other.o = nullptr;
379-
incref_if_owned<owns_ref && !other_owns_ref, not_null>(o);
378+
// If the type being assigned to does not own its own reference,
379+
// decref the wrapped pointer.
380+
decref_if_owned<!owns_ref, false>(o);
381+
// Assert that the wrapped reference is still valid after
382+
// any needed decrefs.
383+
assert(Py_REFCNT(o) > 0);
380384
return *this;
381385
}
382386
else {

0 commit comments

Comments
 (0)