cogciprocate / ocl

OpenCL for Rust
Other
731 stars 75 forks source link

Fix safety for create_context and build/compile/link_program #134

Closed romankoblov closed 5 years ago

romankoblov commented 5 years ago

Hello! There is safety issue on current code, for example for create_context it has signature like that: pub fn create_context<D: core::ClDeviceIdPtr>(device_ids: &[D]) and later it pass device_ids.as_ptr() unmodified to FFI. Problem that both &[DeviceId] and &[&DeviceId] will bypass this signature, and in later case we will have array of pointers to pointers (instead of array of pointers).

Example use case when it can happen:

    let platform = ocl::Platform::default();
    let device_ocl = ocl::Device::first(platform).unwrap();

    let device_ids = core::get_device_ids(&platform, None, None).unwrap();
    let device = device_ids[0];
    let device_ocl = device_ocl.as_core(); // returns &DeviceID

    create_context(&[device]); // &[DeviceId]
    create_context(&[device_ocl]); // &[&DeviceId]
    create_context(&[*device_ocl]); // &[DeviceId]

In Linux this still will work (probably there is no check if device is valid for this ctx), but fails in MacOS.

c0gent commented 5 years ago

Very interesting. I think your solution is probably good but let me think about this a bit more.

romankoblov commented 5 years ago

It is possible to force (or convert only when needed) this via type system like this:

pub trait DeviceIdList {}
impl DeviceIdList for &[core::DeviceId; 1] {}
//impl DeviceIdList for &[&core::DeviceId; 1] { /* do conversion */ }

pub fn create_context2<D: DeviceIdList>(device_ids: D){}

create_context2(&[device]); // &[DeviceId] - success
create_context2(&[device_ocl]); // &[&DeviceId]  - fails
create_context2(&[*device_ocl]); // &[DeviceId] - success

But since context/creation is pretty rare operation and in case of compilation it will be slow enough (so allocation will be unnoticed), I think it will add unnecessary complexity.

c0gent commented 5 years ago

I've submitted a Pull request to your branch which includes a slightly safer solution. Please give that a test and let me know if you have any feedback about it. Merge to your branch if it looks good.

romankoblov commented 5 years ago

I've checked on MacOS && Linux, works fine. Looks a lot better than my solution :)

c0gent commented 5 years ago

Great. Thanks for the contribution!