Use arrays instead of slices and Vecs #33
Conversation
|
I don't remember why it was done this way. I suspect that maybe Rust had issues with arrays being passed like this? It was losing the size, or maybe it was just very very annoying to use it with fixed sizes because passing an array of a fixed size is a pain? Does this make sense? Can you maybe try using this branch with libetebase and see whether this makes usage very annoying? |
|
Ah, you think the FFI bindings will break? Well, this doesn't actually change any public APIs yet, so that shouldn't be a problem, but I'll test what happens if I do that. Rust itself should be able to handle arrays just fine, the only possible concern is that passing huge arrays on the stack requires a bunch of copying, but since all the arrays here are <= 32 bytes, that shouldn't be a problem. |
No, libetebase wraps around this lib. I just remember it was very painful to e.g. take a Vec and pass it to a sized array (I think that was the issue). So e.g.: fn foo(bar: &[u8; 32]) {
// ...
}
let a: Vec<u8> = ...;
# Calling foo with a was a pain.I think that was the issue, but I don't remember. What I was saying though: let's also change libetebase to use this updated API and see whether it's very painful, or whether it's easy to use this new API.
Yeah, that's not what I meant. |
That should just be
There is nothing to change in libetebase yet. This PR does not change the public API. Eventually, the public API can be changed to use arrays so that some length checks inside the library can be moved to its consumers - I quickly did that for testing, the libetebase changes are pretty contained: Xiretza/libetebase@37eabc1 |
|
Oh, I didn't realize it didn't change the public API. OK, please rebase over main and I'll take a look. |
|
Hi! It's been a month, just want to make sure this hasn't fallen through the cracks. I don't want to be pushy, but I'd like to get a couple small internal cleanups like this out of the way before I tackle more useful things such as #27. |
|
I don't get notifications unless your re-request a review. So I didn't know you rebased. :) Looking now... |
src/service.rs
Outdated
| // Use only first 16 bytes of salt - previous versions generated a 32-byte salt during | ||
| // signup, but only used the first 16 bytes | ||
| let salt = login_challenge | ||
| .salt | ||
| .get(..SALT_LENGTH) | ||
| .ok_or(Error::Encryption( | ||
| "Salt obtained from login challenge too short: expected at least 16 bytes", | ||
| ))? | ||
| .try_into() | ||
| .unwrap(); | ||
| let main_key = derive_key(&salt, password)?; |
There was a problem hiding this comment.
I think it's probably fine, though it's harmless to keep it at 32 and much less scary.
There was a problem hiding this comment.
This isn't actually a change in functionality, the check has just been moved out of derive_key() (and now doesn't panic anymore if the received hash is shorter than 16 bytes). I've adjusted the comment though.
Doh! Good to know for the future. |
|
Code looks much much better, thanks and gj! |
There are several instance of
&[u8]andVec<u8>being used as argument types for data that is then fallibly converted into a fixed-size array inside the function. Using&[u8; N]or[u8; N]for the argument types instead means that many runtime length checks can be moved from the library's internals all the way out to the API boundary, and eventually removed entirely when the signatures of API functions are updated accordingly in a major update.