smuellerDD / leancrypto

Lean cryptographic library usable for bare-metal environments
https://leancrypto.org
Other
28 stars 3 forks source link

Expensive interfaces in rust #6

Closed simo5 closed 1 month ago

simo5 commented 1 month ago

Hey Stephan, not sure how you prefer feedback, I would open normally a discussion for this kind of thing, but discussions seem not enabled in this repo.

I was taking a quick look at the rust lib and saw this: https://github.com/smuellerDD/leancrypto/blob/c09c4c9476736f59b5107e7ae3cdc05ab7d1e23d/rust/src/lib.rs#L26C1-L26C44

This kind of API is quite expensive, as you are allocating on the stack a buffer of 64 bytes, zeroing it, then filling it, and then copying it out to the caller's stack when the operation is done.

An interface like the following would allow the application to optimize the code by passing it a preallocate buffer once and not making multiple copies in memory: pub fn lcr_sha3_512(msg: &[u8], out: &mut [u8])

The function would have to check that the passed in slice is of the correct length of course.

Alternatively (or in addition) the following would allow you to use memory on the heap and again avoid copies, but will require a heap allocation (two options): a) pub fn lcr_sha3_512(msg: &[u8], out: &mut Vec<u8>) b) pub fn lcr_sha3_512(msg: &[u8]) -> Vec<u8>

HTH

smuellerDD commented 1 month ago

Am Freitag, 27. September 2024, 17:16:39 MESZ schrieb Simo Sorce:

Hi Simo,

Hey Stephan, not sure how you prefer feedback, I would open normally a discussion for this kind of thing, but discussions seem not enabled in this repo.

Ok, I am not aware of these more "fancy" mechanisms - let me check how to enable it :-)

I was taking a quick look at the rust lib and saw this: https://github.com/smuellerDD/leancrypto/blob/c09c4c9476736f59b5107e7ae3cdc0 5ab7d1e23d/rust/src/lib.rs#L26C1-L26C44

This kind of API is quite expensive, as you are allocating on the stack a buffer of 64 bytes, zeroing it, then filling it, and then copying it out to the caller's stack when the operation is done.

An interface like the following would allow the application to optimize the code by passing it a preallocate buffer once and not making multiple copies in memory: pub fn lcr_sha3_512(msg: &[u8], out: &mut [u8])

The function would have to check that the passed in slice is of the correct length of course.

Alternatively (or in addition) the following would allow you to use memory on the heap and again avoid copies, but will require a heap allocation (two options): a) pub fn lcr_sha3_512(msg: &[u8], out: &mut Vec<u8>) b) pub fn lcr_sha3_512(msg: &[u8]) -> Vec<u8>

Thanks for the help, but I do not consider this API as production-ready. I started with this out of frustation that the inline functions in the API header files are not RUST-wrapped automatically.

HTH

Ciao Stephan

simo5 commented 1 month ago

Well the inline functions can't be wrapped, because you are telling he compiler to inline them, something rust has no concept for, and also something that will make the ABI break easily. Public functions should never be declared via inlining in headers, and non public stuff is something you do not export via bindings.

My suggestion would be to simply make those regular functions exported by your code and with a stable ABI and not inline them in headers. LTO will take care of optimizing them for builds where you care for it, and will not when not needed.

smuellerDD commented 1 month ago

Am Freitag, 27. September 2024, 17:33:59 MESZ schrieb Simo Sorce:

Hi Simo,

Well the inline functions can't be wrapped, because you are telling he compiler to inline them, something rust has no concept for, and also something that will make the ABI break easily. Public functions should never be declared via inlining in headers, and non public stuff is something you do not export via bindings.

My suggestion would be to simply make those regular functions exported by your code and with a stable ABI and not inline them in headers. LTO will take care of optimizing them for builds where you care for it, and will not when not needed.

That is a point I was mulling back and forth in my head - whether to leave the code in a header file or move it into a C file. The header file with the inlines just provide some "eye candy" or ease-of-use interface - e.g. lc_dilithium.h. This was the reason to leave it as inlines.

If you look at that, what do you think - shall I move it to a C file?

Ciao Stephan

simo5 commented 1 month ago

It is a judgment call, if it is something you expect people can easily implement themselves then you can leave it as an optional helper for C applications only in the header. If you think it would be cumbersome to have to reimplement it then it would be better move to a utility .c file and not inline in a header.

simo5 commented 1 month ago

In the former case you would then either not provide it for rust I would guess

smuellerDD commented 1 month ago

Am Freitag, 27. September 2024, 17:56:38 MESZ schrieb Simo Sorce:

Hi Simo,

It is a judgment call, if it is something you expect people can easily implement themselves then you can leave it as an optional helper for C applications only in the header. If you think it would be cumbersome to have to reimplement it then it would be better move to a utility .c file and not inline in a header.

And I am at that decision point since I implemented it :-)

Let me think about that a bit more over the weekend.

Ciao Stephan

smuellerDD commented 1 month ago

All APIs are now in C. Thus, the RUST example is not needed any more. Could I ask you for one simple example now that could replace the examples/hash_example/src/main.rs and perhaps remove lib.rs?

simo5 commented 1 month ago

Are you asking for an example crate about how to use this library with autogenerated rust bindings?

smuellerDD commented 1 month ago

Am Montag, 30. September 2024, 16:44:27 MESZ schrieb Simo Sorce:

Hi Simo,

Are you asking for an example crate about how to use this library with autogenerated rust bindings?

Perhaps, that would be awesome.

With the mentioned change, I can now remove the expensive interface you rightfully complained about.

Ciao Stephan

simo5 commented 1 month ago

I think you should just drop the current stuff for now. I might find some time to provide you an example, but I can't promise. (I definitely won't do it today, I've done three changes to software today and broke 5 tests :-) not the right day to adventure in code writing I guess )

smuellerDD commented 1 month ago

Am Montag, 30. September 2024, 20:05:01 MESZ schrieb Simo Sorce:

Hi Simo,

I think you should just drop the current stuff for now.

Will do, thanks a lot for your comments.

I might find some time to provide you an example, but I can't promise. (I definitely won't do it today, I've done three changes to software today and broke 5 tests :-) not the right day to adventure in code writing I guess )

Sure sure, take your time. And if I find time, I will add it too.

Ciao Stephan

smuellerDD commented 1 month ago

Offending code removed.

smuellerDD commented 1 month ago

BTW I have activated discussions.