`Tensor::get_byte_size` returns incorrect result with `v2024.4.0`
abrown opened this issue · 2 comments
TLDR: this issue describes how an breaking change in the upstream C bindings results in incorrect behavior in these Rust bindings; the solution is to use a newer version of OpenVINO (v2024.2 and up) along with a recent version of these bindings (v0.8.0 and up).
#142 adds an easier way to update the bindings to use the latest OpenVINO C headers; as I used it I discovered the Tensor::get_byte_size
is returning a different result than previously. I ran:
$ cargo xtask update 2024.4.0
$ cargo xtask codegen
$ cargo test
And the memory_safety
test promptly failed with:
thread 'memory_safety' panicked at crates/openvino/tests/memory-safety.rs:26:40:
source slice length (13956476) does not match destination slice length (111651808)
Interestingly enough, 111651808 / 13956476 = 8
so it could be that some internal data type was unwittingly changed upstream. It's notable that the weights
buffer we pass in contains 13956476 bytes and has had that size since the mobilenet
test fixture was added. And I am running the test with the OpenVINO 2024.1.0 library installed; the only thing that changes here is the OpenVINO C header.
I investigated this and the cause is the upstream commit at openvinotoolkit/openvino@0d95325972f (PR: openvinotoolkit/openvino#24608). What happened is that three new enum variants (U2
, U3
, U6
) were inserted prior to the U8
variant, pushing its discriminant value (14
) to that of U64
(17
). The commit says it is part of releases v2024.2.0 and following, so if someone used the library at v2024.1.0 and below (like I did above) with bindings for v2024.2.0 and above (which is what my update did) they would see this issue.
This raises a versioning problem: a minor version bump should be backwards compatible (like adding enum variants to the end of the list!) but this change was not. But the v<year>.<version>.<patch>
scheme that OpenVINO seems to be using is probably not intended for semver compatibility anyways. So, for this crate, let's bump our major version (which still lives in the minor version placeholder... 1.0 in 2025?) and document that the ElementType
enum will break on older versions of OpenVINO. For the memory-safety.rs
test, let's see if we can't ignore it if we check that the version is older than v2024.2.0.
@rahulchaphalkar, thinking about this more, I'm not as worried about a security issue here: there are three parts to this puzzle — the user, the bindings, and the OpenVINO library. If we're using pre-2024.2 bindings with a post-2024.2 library (or vice versa) there is a mismatch between what the user asks for (e.g., U8
) and what the library sees (e.g., U64
). I was worried that there would be a mismatch between the bindings and the library, but no, they will always be in sync; if they weren't, we could get the case where Tensor::get_raw_data_mut
could allow access to more memory than it should. But this won't happen because Tensor
always asks OpenVINO what size it should be: the call to ov_tensor_get_byte_size
always returns the correct OpenVINO-side size, regardless of if the element type is confused in the bindings. Now, the user may still be very confused, but I think we can rule out a memory safety issue.