Twinklebear / oidn-rs

Rust bindings to Intel's OpenImageDenoise Library
MIT License
24 stars 12 forks source link

Use usize for size_t #21

Closed khang06 closed 1 year ago

khang06 commented 1 year ago

Fixes #19.

The bindings were incorrectly using an unsigned 32-bit integer for size_t, causing the compiler to only set the low 32 bits of any arguments using that type. OIDN would then break after interpreting the argument as a really large 64-bit integer because the upper 32 bits aren't cleared.

Twinklebear commented 1 year ago

Nice find @khang06 ! This is in the bindgen generated header, is there a way to configure bindgen to use usize for size_t instead of ulong? It seems like this would only be an issue on 32-bit builds which is interesting too, maybe that's why it was tough to reproduce

Twinklebear commented 1 year ago

It looks like there's a flag for bindgen --size_t-is-usize although maybe using usize is the default now, looking at bindgen --help, I now just see a flag to unset usize being used for size_t

khang06 commented 1 year ago

It seems like this would only be an issue on 32-bit builds which is interesting too, maybe that's why it was tough to reproduce

It's actually the opposite case. It theoretically would've worked fine on 32-bit builds because their size_t is 32-bit. The reason it's hard to reproduce is because the contents of the upper 32 bits are undefined when it gets to the call, so whether it crashed or not really depended on what the code that executed right before the call was doing. Maybe debug builds clear that part, but I haven't checked.