use getptr a lot less (#618)

* use getptr a lot less

* Don't fail fast

* typo

* Fix: Use GC.@preserve in pyconvert_rule_ctypessimplevalue

* preserve in incref/decref (use-after-free)

* more preserve

* more preserve

---------

Co-authored-by: Christopher Doris <github.com/cjdoris>

Author: cjdoris

.github/workflows/tests.yml

@@ -15,7 +15,7 @@ jobs:
     name: Test (${{ matrix.os }}, julia ${{ matrix.jlversion }})
     runs-on: ${{ matrix.os }}
     strategy:
-      fail-fast: true
+      fail-fast: false
       matrix:
         arch: [x64] # x86 unsupported by MicroMamba
         os: [ubuntu-latest, windows-latest, macos-latest]
@@ -53,7 +53,7 @@ jobs:
     name: Test (${{ matrix.os }}, python ${{ matrix.pyversion }})
     runs-on: ${{ matrix.os }}
     strategy:
-      fail-fast: true
+      fail-fast: false
       matrix:
         os: [ubuntu-latest, windows-latest, macos-latest]
         pyversion: [">=3.8", "3.8"]

src/C/extras.jl

@@ -1,23 +1,26 @@
-Py_Type(x::PyPtr) = PyPtr(UnsafePtr(x).type[!])
+asptr(x) = Base.unsafe_convert(PyPtr, x)
 
-PyObject_Type(x::PyPtr) = (t = Py_Type(x); Py_IncRef(t); t)
+Py_Type(x) = Base.GC.@preserve x PyPtr(UnsafePtr(asptr(x)).type[!])
 
-Py_TypeCheck(o::PyPtr, t::PyPtr) = PyType_IsSubtype(Py_Type(o), t)
-Py_TypeCheckFast(o::PyPtr, f::Integer) = PyType_IsSubtypeFast(Py_Type(o), f)
+PyObject_Type(x) = Base.GC.@preserve x (t = Py_Type(asptr(x)); Py_IncRef(t); t)
 
-PyType_IsSubtypeFast(t::PyPtr, f::Integer) =
-    Cint(!iszero(UnsafePtr{PyTypeObject}(t).flags[] & f))
+Py_TypeCheck(o, t) = Base.GC.@preserve o t PyType_IsSubtype(Py_Type(asptr(o)), asptr(t))
+Py_TypeCheckFast(o, f::Integer) = Base.GC.@preserve o PyType_IsSubtypeFast(Py_Type(asptr(o)), f)
 
-PyMemoryView_GET_BUFFER(m::PyPtr) = Ptr{Py_buffer}(UnsafePtr{PyMemoryViewObject}(m).view)
+PyType_IsSubtypeFast(t, f::Integer) =
+    Base.GC.@preserve t Cint(!iszero(UnsafePtr{PyTypeObject}(asptr(t)).flags[] & f))
 
-PyType_CheckBuffer(t::PyPtr) = begin
-    p = UnsafePtr{PyTypeObject}(t).as_buffer[]
+PyMemoryView_GET_BUFFER(m) = Base.GC.@preserve m Ptr{Py_buffer}(UnsafePtr{PyMemoryViewObject}(asptr(m)).view)
+
+PyType_CheckBuffer(t) = Base.GC.@preserve t begin
+    p = UnsafePtr{PyTypeObject}(asptr(t)).as_buffer[]
     return p != C_NULL && p.get[!] != C_NULL
 end
 
-PyObject_CheckBuffer(o::PyPtr) = PyType_CheckBuffer(Py_Type(o))
+PyObject_CheckBuffer(o) = Base.GC.@preserve o PyType_CheckBuffer(Py_Type(asptr(o)))
 
-PyObject_GetBuffer(o::PyPtr, b, flags) = begin
+PyObject_GetBuffer(_o, b, flags) = Base.GC.@preserve _o begin
+    o = asptr(_o)
     p = UnsafePtr{PyTypeObject}(Py_Type(o)).as_buffer[]
     if p == C_NULL || p.get[!] == C_NULL
         PyErr_SetString(
@@ -61,8 +64,8 @@ function PyOS_RunInputHook()
     end
 end
 
-function PySimpleObject_GetValue(::Type{T}, o::PyPtr) where {T}
-    UnsafePtr{PySimpleObject{T}}(o).value[!]
+function PySimpleObject_GetValue(::Type{T}, o) where {T}
+    Base.GC.@preserve o UnsafePtr{PySimpleObject{T}}(asptr(o)).value[!]
 end
 
 # FAST REFCOUNTING

src/Compat/pycall.jl

@@ -16,6 +16,6 @@ function init_pycall(PyCall::Module)
     end
     @eval function PyCall.PyObject(x::Py)
         C.CTX.matches_pycall::Bool || error($errmsg)
-        return $PyCall.PyObject($PyCall.PyPtr(incref(getptr(x))))
+        return $PyCall.PyObject($PyCall.PyPtr(getptr(incref(x))))
     end
 end

src/Convert/ctypes.jl

@@ -1,12 +1,14 @@
 struct pyconvert_rule_ctypessimplevalue{R,S} <: Function end
 
 function (::pyconvert_rule_ctypessimplevalue{R,SAFE})(::Type{T}, x::Py) where {R,SAFE,T}
-    ptr = Base.GC.@preserve x C.PySimpleObject_GetValue(Ptr{R}, getptr(x))
-    ans = unsafe_load(ptr)
-    if SAFE
-        pyconvert_return(convert(T, ans))
-    else
-        pyconvert_tryconvert(T, ans)
+    Base.GC.@preserve x begin
+        ptr = C.PySimpleObject_GetValue(Ptr{R}, x)
+        ans = unsafe_load(ptr)
+        if SAFE
+            pyconvert_return(convert(T, ans))
+        else
+            pyconvert_tryconvert(T, ans)
+        end
     end
 end
 

src/Convert/numpy.jl

@@ -1,7 +1,7 @@
 struct pyconvert_rule_numpysimplevalue{R,S} <: Function end
 
 function (::pyconvert_rule_numpysimplevalue{R,SAFE})(::Type{T}, x::Py) where {R,SAFE,T}
-    ans = Base.GC.@preserve x C.PySimpleObject_GetValue(R, getptr(x))
+    ans = C.PySimpleObject_GetValue(R, x)
     if SAFE
         pyconvert_return(convert(T, ans))
     else

src/Convert/pyconvert.jl

@@ -238,7 +238,7 @@ function _pyconvert_get_rules(pytype::Py)
         end
     end
     for (t, x) in reverse(collect(zip(mro, xmro)))
-        if C.PyType_CheckBuffer(getptr(t))
+        if C.PyType_CheckBuffer(t)
             push!(x, "<buffer>")
             break
         end
@@ -350,7 +350,7 @@ function pytryconvert(::Type{T}, x_) where {T}
 
     # get rules from the cache
     # TODO: we should hold weak references and clear the cache if types get deleted
-    tptr = C.Py_Type(getptr(x))
+    tptr = C.Py_Type(x)
     trules = pyconvert_rules_cache(T)
     rules = get!(trules, tptr) do
         t = pynew(incref(tptr))

src/Convert/rules.jl

@@ -61,8 +61,8 @@ pyconvert_rule_bytes(::Type{Base.CodeUnits{UInt8,String}}, x::Py) =
 pyconvert_rule_int(::Type{T}, x::Py) where {T<:Number} = begin
     # first try to convert to Clonglong (or Culonglong if unsigned)
     v =
-        T <: Unsigned ? C.PyLong_AsUnsignedLongLong(getptr(x)) :
-        C.PyLong_AsLongLong(getptr(x))
+        T <: Unsigned ? C.PyLong_AsUnsignedLongLong(x) :
+        C.PyLong_AsLongLong(x)
     if !iserrset_ambig(v)
         # success
         return pyconvert_tryconvert(T, v)

src/Core/Py.jl

@@ -51,6 +51,11 @@ pyconvert(::Type{Py}, x::Py) = x
 
 setptr!(x::Py, ptr::C.PyPtr) = (setfield!(x, :ptr, ptr); x)
 
+incref(x::Py) = Base.GC.@preserve x (incref(getptr(x)); x)
+decref(x::Py) = Base.GC.@preserve x (decref(getptr(x)); x)
+
+Base.unsafe_convert(::Type{C.PyPtr}, x::Py) = getptr(x)
+
 const PYNULL_CACHE = Py[]
 
 """
@@ -75,7 +80,7 @@ const PyNULL = pynew()
 
 pynew(ptr::C.PyPtr) = setptr!(pynew(), ptr)
 
-pynew(x::Py) = pynew(incref(getptr(x)))
+pynew(x::Py) = Base.GC.@preserve x pynew(incref(getptr(x)))
 
 """
     pycopy!(dst::Py, src)
@@ -164,13 +169,13 @@ Base.print(io::IO, x::Py) = print(io, string(x))
 
 function Base.show(io::IO, x::Py)
     if get(io, :typeinfo, Any) == Py
-        if getptr(x) == C.PyNULL
+        if pyisnull(x)
             print(io, "NULL")
         else
             print(io, pyrepr(String, x))
         end
     else
-        if getptr(x) == C.PyNULL
+        if pyisnull(x)
             print(io, "<py NULL>")
         else
             s = pyrepr(String, x)