rust-keyutils icon indicating copy to clipboard operation
rust-keyutils copied to clipboard

{Key, Keyring}::new should (probably) not be unsafe

Open Nemo157 opened this issue 4 years ago • 2 comments

As far as I can tell inventing an invalid key id will just cause errors on future API calls, there is no way to trigger memory unsafety, so this function should not be marked unsafe. To reduce the chance of users using this incorrectly it could be renamed to new_unchecked to highlight that it's up to the user to ensure they use a valid id.

Nemo157 avatar Dec 31 '20 11:12 Nemo157

https://doc.rust-lang.org/std/num/struct.NonZeroI32.html#method.new_unchecked is unsafe. I do like it better as a name, but the thing is that there's no way to know if a given ID is valid or not on our side to panic! if it is invalid (not to mention inherent TOCTOU issues).

mathstuf avatar Dec 31 '20 20:12 mathstuf

Hmm, I thought there was some convention for safe _unchecked functions, but looks like I was misremembering. In that case, just remove the unsafe and clarify in the docs that an invalid id will likely lead to future errors?

Nemo157 avatar Jan 01 '21 09:01 Nemo157