Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ort::value::Value is unsound since it exposes raw pointers as public fields #69

Closed
SludgePhD opened this issue Jul 25, 2023 · 1 comment · Fixed by #78
Closed

ort::value::Value is unsound since it exposes raw pointers as public fields #69

SludgePhD opened this issue Jul 25, 2023 · 1 comment · Fixed by #78
Assignees
Labels
p: high high priority

Comments

@SludgePhD
Copy link

The Value type is defined like this:

pub enum Value<'v> {
	RustOwned {
		ptr: *mut sys::OrtValue,
		array: DynArrayRef<'v>,
		memory_info: MemoryInfo
	},
	CppOwned {
		ptr: *mut sys::OrtValue,
		session: Arc<SessionPointerHolder>
	}
}

Enum variants and their fields are public, so downstream code can:

  • construct a Value from an arbitrary dangling pointer, or
  • construct a valid Value using the API, and then change the contained pointer to point whereever

None of those things require unsafe code, so this API is unsound. It could be fixed by wrapping the enum in another struct type, as a private field.

@decahedron1 decahedron1 self-assigned this Jul 25, 2023
@decahedron1 decahedron1 added the p: high high priority label Jul 25, 2023
@SludgePhD
Copy link
Author

Oh nevermind, apparently the entire code base is unsound in similar ways, seemingly intentionally. What exactly is the intended way to... interpret this library? Is there a higher-level wrapper around ort that I should be using instead?

@decahedron1 decahedron1 moved this to Todo in ort v2.0 Aug 5, 2023
@decahedron1 decahedron1 moved this from Todo to In Progress in ort v2.0 Aug 5, 2023
@decahedron1 decahedron1 moved this from In Progress to Done in ort v2.0 Aug 14, 2023
@decahedron1 decahedron1 mentioned this issue Aug 16, 2023
Merged
8 tasks
@decahedron1 decahedron1 linked a pull request Aug 20, 2023 that will close this issue
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: high high priority
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants