Wrapping a C lib, question on the safety of convert that uses unsafe_convert to retrieve a pointer

General Usage
1

The following pseudo-code represents a reduced example of a library I am wrapping. The code actually works well, however I think that there are exceptional cases where it is potentially unsafe (see comments below where the convert function is defined).

It may very well be that there is nothing wrong with the code below. I welcome feedback.



# the C library has a struct of this layout
struct CstructX
	w::Cint
	h::Cint
	p::Ptr{UInt8}
end


function Base.convert(::Type{CstructX}, A::AbstractMatrix{NTuple{4,UInt8}})
    # potentially unsafe  ?
	CstructX(size(A)...,  Base.unsafe_convert(Ptr{UInt8}, A))  # yes we want to interpret the array of NTuple{4,UInt8} as   Ptr{UInt8}
end


# the C function expects a pointer to a vector of structs with the CstructX layout
function cwrapper(A::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	Aconv = unsafe_convert(Vector{CstructX}, A)
	ccall((:test), Cvoid, (Cint, Ptr{Vector{CstructX}}), length(A), Aconv)
end


# pseudo code to call the cwrapper

a = rand(UInt8, 24, 24)
a = reinterpret(NTuple{4,UInt8}, A)

A = [a, a, a]
callcfunction(a)
2

Unsafe by it’s own. Wrong to be convert.
It can be more correct as a unsafe_convert definition. It should be correct if you define the corresponding cconvert to call the cconvert for A.

Calling unsafe_convert like this outside of unsafe_convert is basically always wrong. I highly doubt that the C code actually want Ptr{Vector{CstructX}} unless it’s already julia aware. You should define cconvert and unsafe_convert for type Ptr{Ptr{CstructX}} and cconvert input type Vector{<:AbstractMatrix{NTuple{4,UInt8}}} with unspecified unsafe_convert input type.

2 Likes
3

Thanks for the detailed reply.
Actually I initially pursued the unsafe_convert/ cconvert approach, but ran into bugs. I’ll try again and report back.

I highly doubt that the C code actually want Ptr{Vector{CstructX}}

The C code expects a pointer to a C structure with the same layout as CstructX.

I produced a minimal example for this forum post, since I thought it would be easier to describe; but for reference, the actual call I am working on wrapping is GLFW: Window reference, and my PR GLFW allow passing multiple icons by musm · Pull Request #170 · JuliaGL/GLFW.jl · GitHub to incorporate the call. So I think, in this case, the C library does indeed expect a Ptr{Vector{CstructX}}

4

No it’s a Ptr{CstructX}. No C function that’s not julia aware will ever expect a Ptr{Vector{CstructX}}.

2 Likes
5

Do you mean Ptr{CstructX} instead of Ptr{Ptr{CstructX}}? Otherwise I’m confused by the method signatures I should define.

If I understand your comment. Do you mean I need the following signatures defined

Base.cconvert(::Type{Ptr{CstructX}} , data::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})

Base.unsafe_convert(::Type{Ptr{CstructX}} , data)
6

From my understanding it seems like you are suggesting something like the following

function Base.cconvert(::Type{Ptr{CstructX}}, image::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	out = Vector{Tuple{Int, Int, Matrix{NTuple{4,UInt8}}}}(undef,length(image))
	for i = 1:length(image)
		out[i] = (size(image[i])..., image[i])
	end
	out
end

function Base.unsafe_convert(::Type{Ptr{CstructX}}, data::Vector{Tuple{Int, Int, Matrix{NTuple{4,UInt8}}}})
	out = Vector{GLFWImage}(undef,length(data))
	for i = 1:length(data)
		out[i] = CstructX(data[i][1],data[i][2], Base.unsafe_convert(Ptr{UInt8}, data[i][3]))
	end
	pointer(out)
end

This kinds feels wrong since it allocates more memory then it should…

It seems like the following is sufficient and safe?


function Base.cconvert(::Type{Ptr{CstructX}}, image::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	out = Vector{CstructX}(undef,length(image))
	@inbounds for i = 1:length(image)
		out[i] = CstructX(size(image[i])..., Base.unsafe_convert(Ptr{UInt8}, image[i]))
	end
	out
end

however, it does involve an unsafe_convert within a cconvert call…

My final code looks like the following

function Base.cconvert(::Type{Ref{CstructX}}, image::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	out = Vector{CstructX}(undef,length(image))
	@inbounds for i in 1:length(image)
		out[i] = Base.cconvert(CstructX, image[i])
	end
	return out
end

function Base.cconvert(::Type{CstructX}, image::AbstractMatrix{NTuple{4,UInt8}})
	CstructX(size(image)..., Base.unsafe_convert(Ptr{UInt8}, image))
end

function cwrapper(A::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	ccall((:test), Cvoid, (Cint, Ref{CstructX}), length(A), Aconv)
end

Above I am using Ref instead of Ptr for the cconvert override since I am referring to julia managed memory.

7

Yes.

I was thinking about the pointer you have in CstructX and typed one more Ptr…

Yes. The second one could restrict the signature a little bit if necessary and if you could. It just need to be ablt to take the return value of the cconvert.

No. You must return image as well from cconvert. In principle you also must return the cconvert value of image but you can omit that if you don’t need to deal with all AbstractMatrix.

8

So I suppose the following is your suggestion?

function Base.cconvert(::Type{Ptr{CstructX}}, images::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	out = Vector{Tuple{Int,Int,AbstractMatrix{NTuple{4,UInt8}}}}(undef,length(images))
	@inbounds for i in 1:length(images)
		out[i] = (size(images[i])..., images[i))
	end
	return out
end

function Base.unsafe_convert(::Type{Ptr{CstructX}}, data::Vector{<:Tuple{Integer,Integer,AbstractMatrix{NTuple{4,UInt8}}}})
	out = Vector{GLFWImage}(undef, length(data))
	for i = 1:length(data)
		out[i] = CstructX(data[i][1],data[i][2], Base.unsafe_convert(Ptr{UInt8}, data[i][3]))
	end
	return pointer(out)
end

Although I have to admit, this seems suboptimal, since it allocates several arrays in the conversion process…

BTW, shouldn’t these calls be Ref not Ptr since we refer to julia managed memory.

9

Errrr, no. this is even more wrong… There shouldn’t be any guess work… All what you need to do make sure is that all the ponters you want to keep alive belongs to the object (tree) you returned in the cconvert. On top of this, if you want to do generic programming, you can take advantage of this garantee by calling cconvert and unsafe_convert on the “generic” objects. You need to forward this guarantee by makeingsure you include cconvert result in your cconvert if you want to use the unsafe_convert result in your unsafe_convert.

Since none of your code is truely generic anyway, I’ll just ignore the part of calling cconvert on the AbstractMatrix. It’s a generic programming concert not a GC/C interface concern anyway even though in this case it does increase allocation.

With that out of the way, you are passing a pointer to an array of CstructX, so make sure that array is allocated in your cconvert and included in the result. (i.e. the latest version violate this). On top of this, your CstructX contains pointers that I assume you want to keep alive so you must make sure the owner of those pointers (the (Abstract)Matrixs) are included in the cconvert result as well. You can achieve this trivially if you ignore the generic programming concern by simply include the images in the cconvert return value as well (like return (out, images)). None of the versions you have above respects this.

So if you don’t need to handle all AbstractMatrix, the only allocation you need is the final Vector{CstructX} in cconvert. There’s no other allocation needed. You simply need to allocation and populate that vector and return it together with the input and being able to handle this return value in your unsafe_convert.

Doesn’t matter.

10

Thanks for the feedback. This is my latest attempt to incorporate your comments.

So the point of keeping the images array along with the output of cconvert, is to prevent the GC from erasing the input images array?

I need to support AbstractArray, since the input is the output of a reinterpret call.

MWE

c (compile as shared lib)

#include <stdlib.h>
#include <stdio.h>
#include <stdint.h>

typedef struct CImage
{
    int width;
    int height;
    uint8_t* pixels;
} CImage;


void foo(int l,const CImage* icons)
{
    int i;
    for (i = 0; i < l; i++)
    {
        printf("item %i\n", i);
        printf("width %i\n", icons[i].width);
        printf("height %i\n", icons[i].height);
        printf("pointer %p\n\n", icons[i].pixels);
    }
}

int main()
{
    uint8_t c[3] = {1, 3, 0};
    uint8_t b[3] = {2, 4, 1};
    CImage images[] = {{2,3,c}, {1,3,b}};
    foo(2, images);
    return 0;
}

julia wrappers


struct CImage
	width::Cint
	height::Cint
	pixel::Ptr{UInt8}
end


# When passing the image directly
function test(image::AbstractMatrix{NTuple{4,UInt8}})
	return ccall((:foo, "fun.dll"), Cvoid, (Cint, Ref{CImage}), 1, image)
end

function Base.cconvert(::Type{Ref{CImage}}, image::AbstractMatrix{NTuple{4,UInt8}})
	imaget = permutedims(image) # c lib expect row major
	return Base.RefValue{CImage}(CImage(size(imaget)..., pointer(imaget)))
end

# When passing a vector of images
function test(images::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	return ccall((:foo, "fun.dll"), Cvoid, (Cint, Ref{CImage}), length(images), images)
end

function Base.cconvert(::Type{Ref{CImage}}, images::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	out = Vector{CImage}(undef,length(images))
	@inbounds for i in 1:length(images)
		imaget = permutedims(images[i]) # c lib expect row major
		out[i] = CImage(size(imaget)..., pointer(imaget))
	end
	return (out, images)
end

function Base.unsafe_convert(::Type{Ref{CImage}}, data::Tuple{Vector{CImage},Vector{<:AbstractMatrix{NTuple{4,UInt8}}}})
	Base.unsafe_convert(Ref{CImage}, data[1])
end

# test calls

A = rand(UInt32, 12, 24)
B = rand(UInt32, 36, 8)

a = (reinterpret(NTuple{4,UInt8}, A))
aa =  (reinterpret.(NTuple{4,UInt8}, [A, B]))

test(a)
test(aa)
11

Does that look ok/safe ?

12

It’s OK if and only if this line does not allocate a new array.

13

Thanks

Unfortunately permutedims allocates a new array…

Ok I suppose if I call permutedims to allocate new arrays before any ccall’s then everything should finally be safe

e.g.


struct CImage
	width::Cint
	height::Cint
	pixel::Ptr{UInt8}
end


# When passing the image directly
function test(image::AbstractMatrix{NTuple{4,UInt8}})
	imaget = permutedims(image) # c lib expect row major
	return ccall((:foo, "fun.dll"), Cvoid, (Cint, Ref{CImage}), 1, imaget)
end

function Base.cconvert(::Type{Ref{CImage}}, image::AbstractMatrix{NTuple{4,UInt8}})
	return Base.RefValue{CImage}(CImage(size(image)..., pointer(image)))
end

# When passing a vector of images
function test(images::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	imagest = permutedims.(images) # c lib expect row major
	return ccall((:foo, "fun.dll"), Cvoid, (Cint, Ref{CImage}), length(imagest), imagest)
end

function Base.cconvert(::Type{Ref{CImage}}, images::Vector{<:AbstractMatrix{NTuple{4,UInt8}}})
	out = Vector{CImage}(undef,length(images))
	@inbounds for i in 1:length(images)
		out[i] = CImage(size(images[i])..., pointer(images[i]))
	end
	return (out, images)
end

function Base.unsafe_convert(::Type{Ref{CImage}}, data::Tuple{Vector{CImage},Vector{<:AbstractMatrix{NTuple{4,UInt8}}}})
	Base.unsafe_convert(Ref{CImage}, data[1])
end

# test calls

A = rand(UInt32, 12, 24)
B = rand(UInt32, 36, 8)

a = (reinterpret(NTuple{4,UInt8}, A))
aa =  (reinterpret.(NTuple{4,UInt8}, [A, B]))

test(a)

test(aa)
14

This seems fine. Allocating in cconvert is also fine, you just need to include it in the cconvert return value.

1 Like