Defer pointer extraction to unsafe_convert (fixes #41 properly).

The old `MDBValue(::String)` / `MDBValue(::AbstractArray)` /
`MDBValue(::Base.RefValue)` constructors called `Base.unsafe_convert`
to take a raw data pointer eagerly, then stored it in the returned
`MDB_val` struct. The pointer was correctly rooted *in practice*
because the caller's `MDBArg` wrapper held the buffer, but the call
to `unsafe_convert` happened outside of any preservation scope —
violating the Julia GC contract that `unsafe_convert` should only
produce a pointer while ccall is preserving the source.

Push the extraction into a small set of `unsafe_convert(::Ptr{MDB_val},
::MDBArg{D})` methods (one per `D`). These run while ccall is
preserving the `MDBArg`, so the buffer is provably alive at the
moment we ask it for a pointer.

Drop the unsafe `MDBValue(::String)` etc. constructors; the iterator's
buffer-to-MDB_val needs are served by a private `_mdb_val_for(::Vector)`
that's only called inside an explicit `GC.@preserve`. The cursor
iterator now fills `mdb_key_ref` lazily inside the same preserve
block that wraps the ccall.

Rewrites `test/common.jl` to test the cconvert / unsafe_convert
chain — and along the way demonstrates the unsafety by example: the
test helper has to pass the loaded `MDB_val` to a callback inside
`GC.@preserve`, because reading through `mv_data` after leaving the
preserve scope is exactly the bug the carrier exists to prevent.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: maleadt

src/common.jl

@@ -1,59 +1,76 @@
 const EnvironmentFlags = Unsigned
 
-# Build an `MDB_val` (size + raw data pointer) from a heap-rooted Julia value.
-# The pointer is taken via `Base.unsafe_convert` — the same primitive ccall
-# uses internally to lower `Ref{T}`/`Vector{T}`/`String` arguments. The caller
-# is responsible for keeping `val` alive across the ccall (via `GC.@preserve`),
-# since the resulting `MDB_val` is opaque to GC.
+# Zero-valued `MDB_val` sentinels — used as out-parameters and for the
+# "no value" form of `delete!`. Constructing a non-empty `MDB_val` from a
+# Julia value requires taking a raw pointer into that value, which is only
+# safe when the value is GC-preserved across the eventual ccall. We keep
+# that pointer extraction confined to `unsafe_convert(::Ptr{MDB_val}, ::MDBArg)`
+# below, where ccall's automatic preservation covers the carrier.
 MDBValue() = MDB_val(zero(Csize_t), C_NULL)
 MDBValue(::Nothing) = MDBValue()
-MDBValue(val::String) =
-    MDB_val(Csize_t(sizeof(val)),
-            Ptr{Cvoid}(Base.unsafe_convert(Ptr{UInt8}, val)))
-MDBValue(val::AbstractArray{T}) where {T} =
-    MDB_val(Csize_t(sizeof(T) * length(val)),
-            Ptr{Cvoid}(Base.unsafe_convert(Ptr{T}, val)))
-MDBValue(val::Base.RefValue{T}) where {T} =
-    MDB_val(Csize_t(sizeof(T)),
-            Ptr{Cvoid}(Base.unsafe_convert(Ptr{T}, val)))
 
-# Self-rooted argument for `Ptr{MDB_val}` ccall sites: holds the
-# `Ref{MDB_val}` box and a reference to the data buffer the box's `mv_data`
-# field aliases. Returned from `cconvert` below so ccall's automatic
-# `GC.@preserve` covers both the box and the data — callers never need
-# explicit `Ref(...)`, `MDBValue(...)`, or `GC.@preserve` for input args.
+# Self-rooted argument for `Ptr{MDB_val}` ccall sites. `box` is an
+# uninitialized `Ref{MDB_val}`; `data` is the Julia-owned buffer whose
+# pointer the C call needs to see. `cconvert` returns an `MDBArg`, ccall
+# preserves it across the call, and `unsafe_convert` (below) is the one
+# place pointer extraction happens — that means `data` is provably alive
+# at the moment its pointer is taken.
 struct MDBArg{D}
     box::Base.RefValue{MDB_val}
     data::D
+    MDBArg(data::D) where {D} = new{D}(Ref{MDB_val}(), data)
+end
+
+# Lazy pointer extraction. Runs while ccall is preserving `m`; therefore
+# `m.data` is alive at the point we ask it for a pointer, satisfying the
+# Julia GC contract for `Base.unsafe_convert`.
+@inline function Base.unsafe_convert(::Type{Ptr{MDB_val}}, m::MDBArg{String})
+    m.box[] = MDB_val(Csize_t(sizeof(m.data)),
+                       Ptr{Cvoid}(Base.unsafe_convert(Ptr{UInt8}, m.data)))
+    return Base.unsafe_convert(Ptr{MDB_val}, m.box)
+end
+@inline function Base.unsafe_convert(::Type{Ptr{MDB_val}}, m::MDBArg{<:AbstractArray{T}}) where {T}
+    m.box[] = MDB_val(Csize_t(sizeof(T) * length(m.data)),
+                       Ptr{Cvoid}(Base.unsafe_convert(Ptr{T}, m.data)))
+    return Base.unsafe_convert(Ptr{MDB_val}, m.box)
+end
+@inline function Base.unsafe_convert(::Type{Ptr{MDB_val}}, m::MDBArg{<:Base.RefValue{T}}) where {T}
+    m.box[] = MDB_val(Csize_t(sizeof(T)),
+                       Ptr{Cvoid}(Base.unsafe_convert(Ptr{T}, m.data)))
+    return Base.unsafe_convert(Ptr{MDB_val}, m.box)
 end
-Base.unsafe_convert(::Type{Ptr{MDB_val}}, m::MDBArg) =
-    Base.unsafe_convert(Ptr{MDB_val}, m.box)
 
 # Bare `MDB_val` (used for `delete!`'s empty val): heap-box it.
 Base.cconvert(::Type{Ptr{MDB_val}}, x::MDB_val) = Ref(x)
-# Pre-built `Ref{MDB_val}` (used by iterator state, and as the out-param
-# for `get`/`mdb_cursor_get`): ccall reads/writes the box directly.
+# Pre-built `Ref{MDB_val}` (iterator state, `get` out-param): passthrough;
+# ccall reads/writes the box directly.
 Base.cconvert(::Type{Ptr{MDB_val}}, x::Base.RefValue{MDB_val}) = x
-# User input — heap-rooted forms with stable data pointers.
-Base.cconvert(::Type{Ptr{MDB_val}}, x::String)        = MDBArg(Ref(MDBValue(x)), x)
-Base.cconvert(::Type{Ptr{MDB_val}}, x::Array)         = MDBArg(Ref(MDBValue(x)), x)
+# User input — package the data, defer pointer extraction.
+Base.cconvert(::Type{Ptr{MDB_val}}, x::String)        = MDBArg(x)
+Base.cconvert(::Type{Ptr{MDB_val}}, x::Array)         = MDBArg(x)
 # Other AbstractArrays that support `unsafe_convert(Ptr{T}, x)` flow through —
 # contiguous `SubArray`, `ReinterpretArray`, etc. Non-contiguous inputs
 # surface the standard "cannot take pointer" error from `unsafe_convert`.
-# Matches the scope of `MDBValue(::AbstractArray)` above. The `Array`
-# method above stays as a more-specific overload to break the ambiguity
-# with `Base.cconvert(::Type{<:Ptr}, ::Array)`.
-Base.cconvert(::Type{Ptr{MDB_val}}, x::AbstractArray) = MDBArg(Ref(MDBValue(x)), x)
-Base.cconvert(::Type{Ptr{MDB_val}}, x::Base.RefValue) = MDBArg(Ref(MDBValue(x)), x)
-# User input — bare bitstype scalar. Wrap in a `Ref` to give it a heap
-# address, then build the `MDBArg`. The `Ref` lives in `MDBArg.data` and
-# is rooted by ccall's preserve.
+# The `Array` method above stays as a more-specific overload to break the
+# ambiguity with `Base.cconvert(::Type{<:Ptr}, ::Array)`.
+Base.cconvert(::Type{Ptr{MDB_val}}, x::AbstractArray) = MDBArg(x)
+Base.cconvert(::Type{Ptr{MDB_val}}, x::Base.RefValue) = MDBArg(x)
+# Bare bitstype scalar: heap-box via `Ref` so it has a stable address.
 function Base.cconvert(::Type{Ptr{MDB_val}}, x::T) where {T}
     isbitstype(T) || throw(MethodError(Base.cconvert, (Ptr{MDB_val}, x)))
-    rx = Ref(x)
-    MDBArg(Ref(MDBValue(rx)), rx)
+    MDBArg(Ref(x))
 end
 
+# Private: build an `MDB_val` whose `mv_data` aliases `buf`. The pointer
+# is taken via `unsafe_convert`; the caller MUST keep `buf` alive across
+# any ccall that reads the resulting `MDB_val` (in practice, by wrapping
+# the call site in `GC.@preserve buf ...`). Used by the cursor iterator,
+# which already threads its key buffer through the iteration state for
+# exactly this reason.
+@inline _mdb_val_for(buf::Vector{T}) where {T} =
+    MDB_val(Csize_t(sizeof(T) * length(buf)),
+            Ptr{Cvoid}(Base.unsafe_convert(Ptr{T}, buf)))
+
 mbd_unpack(::Type{T}, mdb_val_ref::Ref{MDB_val}) where {T} = _mbd_unpack(T, mdb_val_ref[])
 function _mbd_unpack(::Type{T}, mdb_val::MDB_val) where {T <: String}
     unsafe_string(convert(Ptr{UInt8}, mdb_val.mv_data), mdb_val.mv_size)

src/cur.jl

@@ -82,22 +82,24 @@ struct ReturnValueSize end
 
 arcopy(x::Array) = copy(x)
 arcopy(x) = x
-# process_returns returns (retval, next_op, key_buf). key_buf is whatever the
-# iterator wrote into mdb_key_ref via mdb_key_ref[] = MDBValue(buf); it must
-# be GC-preserved across the next mdb_cursor_get call. Variants that don't
-# rewrite the key return `nothing` for key_buf.
+# process_returns returns (retval, next_op, key_buf). key_buf is the Julia
+# buffer the next iteration will fill `mdb_key_ref` from; it must outlive
+# the next mdb_cursor_get call. Variants that don't seed a SET_RANGE
+# return `nothing` for key_buf.
 process_returns(::ReturnKeys{K}, mdb_key_ref, _) where K = arcopy(mbd_unpack(K, mdb_key_ref)), MDB_NEXT, nothing
 process_returns(::ReturnValues{V}, _, mdb_val_ref) where V = arcopy(mbd_unpack(V, mdb_val_ref)), MDB_NEXT, nothing
 process_returns(::ReturnBoth{K,V}, mdb_key_ref, mdb_val_ref) where {K,V} = arcopy((mbd_unpack(K, mdb_key_ref)) => arcopy(mbd_unpack(V, mdb_val_ref))), MDB_NEXT, nothing
 process_returns(::ReturnValueSize, _, mdb_val_ref) = mdb_val_ref[].mv_size, MDB_NEXT, nothing
 function init_values(d::LMDBIterator)
-    k, op, key_buf = if !isempty(d.prefix)
-        Ref(MDBValue(d.prefix)), MDB_SET_RANGE, d.prefix
+    if !isempty(d.prefix)
+        # The Ref is initialised lazily inside `iterate`, under
+        # `GC.@preserve key_buf`, so the pointer into `d.prefix` is
+        # only taken when the buffer is provably alive.
+        k = Ref(MDBValue())
+        return k, Ref(MDBValue()), MDB_SET_RANGE, d.prefix
     else
-        Ref(MDBValue()), MDB_FIRST, nothing
+        return Ref(MDBValue()), Ref(MDBValue()), MDB_FIRST, nothing
     end
-    v = Ref(MDBValue())
-    return k, v, op, key_buf
 end
 
 Base.iterate(iter::LMDBIterator) = Base.iterate(iter, init_values(iter))
@@ -106,10 +108,15 @@ Base.iterate(iter::LMDBIterator) = Base.iterate(iter, init_values(iter))
 function Base.iterate(iter::LMDBIterator, refs)
     mdb_key_ref, mdb_val_ref, cursor_op, key_buf = refs
 
-    # key_buf may be aliased by mdb_key_ref (e.g. DirectoryLister or
-    # SET_RANGE seed) and must outlive the C call. unchecked_* because the
-    # iterator branches on MDB_NOTFOUND itself.
-    ret = GC.@preserve key_buf unchecked_mdb_cursor_get(iter.cur, mdb_key_ref, mdb_val_ref, cursor_op)
+    # If we have a key buffer (SET_RANGE seed or DirectoryLister rewrite),
+    # fill `mdb_key_ref` with its pointer under GC.@preserve — the pointer
+    # is only valid while `key_buf` is rooted, and ccall extends that
+    # rooting through the call. unchecked_* because the iterator branches
+    # on MDB_NOTFOUND itself.
+    ret = GC.@preserve key_buf begin
+        key_buf === nothing || (mdb_key_ref[] = _mdb_val_for(key_buf))
+        unchecked_mdb_cursor_get(iter.cur, mdb_key_ref, mdb_val_ref, cursor_op)
+    end
 
     if ret == 0
         #Check if we are still in key prefix
@@ -146,11 +153,16 @@ function process_returns(l::DirectoryLister{K}, mdb_key_ref, _) where K
     else
         k = copy(k)
         resize!(k,nextsep)
-        kout = arcopy(mbd_unpack(K, Ref(MDBValue(k))))
+        # Decode `k` under GC.@preserve — `_mdb_val_for(k)` takes a raw
+        # pointer into `k` that must outlive the `mbd_unpack` read.
+        local kout
+        GC.@preserve k begin
+            kref = Ref(_mdb_val_for(k))
+            kout = arcopy(mbd_unpack(K, kref))
+        end
         k[end] = k[end]+1
-        mdb_key_ref[] = MDBValue(k)
-        # Return k so the next iterate() can GC.@preserve it across the
-        # MDB_SET_RANGE call that reads through mdb_key_ref.
+        # Return `k` as the next iteration's key_buf; `iterate` will
+        # fill `mdb_key_ref` from it under its own GC.@preserve.
         return kout, MDB_SET_RANGE, k
     end
 end

test/common.jl

@@ -1,4 +1,3 @@
-import LMDB.MDBValue
 using LMDB
 using Test
 
@@ -9,49 +8,59 @@ using Test
     @test_throws LMDBError throw(ex)
     @test ex.code == 0
 
-    # MDBValue
-    val = "abcd" # string
-    mdb_val_ref = Ref(MDBValue(val));
-    mdb_val = mdb_val_ref[]
-    # @test val == unsafe_string(convert(Ptr{UInt8}, mdb_val.data), mdb_val.size)
-    @test val == LMDB.mbd_unpack(String, mdb_val_ref)
+    # cconvert / unsafe_convert dispatch. Mirrors what ccall does: takes
+    # an input value, runs the cconvert chain to get a carrier, then
+    # passes the carrier + the resulting `MDB_val` to `f` inside a
+    # `GC.@preserve` so pointer extraction is correctly scoped. We can't
+    # return the `MDB_val` and read its `mv_data` afterwards — that's
+    # exactly the unsafety the carrier exists to prevent.
+    function with_mdb_val(f, x)
+        arg = Base.cconvert(Ptr{LMDB.MDB_val}, x)
+        GC.@preserve arg begin
+            ptr = Base.unsafe_convert(Ptr{LMDB.MDB_val}, arg)
+            f(unsafe_load(ptr))
+        end
+    end
+
+    # String
+    val = "abcd"
+    with_mdb_val(val) do mdb_val
+        @test mdb_val.mv_size == sizeof(val)
+        @test unsafe_string(Ptr{UInt8}(mdb_val.mv_data), mdb_val.mv_size) == val
+    end
 
-    val = [1233] # dense array
+    # Int array
+    val = [1233]
     T = eltype(val)
-    val_size = sizeof(val)
-    mdb_val_ref = Ref(MDBValue(val));
-    mdb_val = mdb_val_ref[]
-    @test val_size == mdb_val.mv_size
-    nvals = floor(Int, mdb_val.mv_size/sizeof(T))
-    value = unsafe_wrap(Array, convert(Ptr{T}, mdb_val.mv_data), nvals)
-    @test val == value
-    @test val == LMDB.mbd_unpack(Vector{Int}, mdb_val_ref)
+    with_mdb_val(val) do mdb_val
+        @test mdb_val.mv_size == sizeof(val)
+        nvals = floor(Int, mdb_val.mv_size / sizeof(T))
+        value = unsafe_wrap(Array, convert(Ptr{T}, mdb_val.mv_data), nvals)
+        @test val == value
+    end
 
+    # UInt16 array
     val = [0x0003, 0xff45]
-    val_size = sizeof(val)
     T = eltype(val)
-    mdb_val_ref = Ref(MDBValue(val));
-    mdb_val = mdb_val_ref[]
-    @test val_size == mdb_val.mv_size
-    nvals = floor(Int, mdb_val.mv_size/sizeof(T))
-    value = unsafe_wrap(Array, convert(Ptr{T}, mdb_val.mv_data), nvals)
-    @test val == value
-    @test val == LMDB.mbd_unpack(Vector{UInt16}, mdb_val_ref)
+    with_mdb_val(val) do mdb_val
+        @test mdb_val.mv_size == sizeof(val)
+        nvals = floor(Int, mdb_val.mv_size / sizeof(T))
+        value = unsafe_wrap(Array, convert(Ptr{T}, mdb_val.mv_data), nvals)
+        @test val == value
+    end
 
+    # Isbits struct: flows through the bitstype cconvert branch
+    # (heap-boxed via `Ref(x)` so it has a stable address).
     struct TestType
         i::Int
         j::Char
     end
-    val = TestType(1,'a') # struct
-    val_size = sizeof(val)
+    val = TestType(1, 'a')
     T = typeof(val)
-    @test_throws MethodError MDBValue(val)
-    val = [val]
-    mdb_val_ref = Ref(MDBValue(val));
-    mdb_val = mdb_val_ref[]
-    @test val_size == mdb_val.mv_size
-    nvals = floor(Int, mdb_val.mv_size/sizeof(T))
-    value = unsafe_wrap(Array, convert(Ptr{T}, mdb_val.mv_data), nvals)
-    @test val == value
-    @test val == LMDB.mbd_unpack(Vector{T}, mdb_val_ref)
-    @test val[1] == LMDB.mbd_unpack(T, mdb_val_ref)
+    with_mdb_val(val) do mdb_val
+        @test mdb_val.mv_size == sizeof(T)
+        @test unsafe_load(Ptr{T}(mdb_val.mv_data)) == val
+    end
+
+    # Non-isbits values have no pointer-stable shape.
+    @test_throws MethodError with_mdb_val(_ -> nothing, (1, [2]))