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

[std::span] Pass std::span buffers into Block_Cipher Implementations #3870

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Jan 3, 2024

This demotes Block_Cipher::encrypt_n/decrypt_n to top-level methods and introduces new (private) virtual methods (encrypt/decrypt_blocks) that use std::span for the in/out buffers. Also, this adapts all block cipher implementations in the library to use the new API.

@reneme reneme added this to the Botan 3.4.0 milestone Jan 3, 2024
@reneme reneme self-assigned this Jan 3, 2024
src/lib/block/seed/seed.cpp Outdated Show resolved Hide resolved
@reneme reneme force-pushed the span/block_cipher_internals branch from 2a75417 to 927ac16 Compare January 3, 2024 18:33
@coveralls
Copy link

coveralls commented Jan 3, 2024

Coverage Status

coverage: 92.089% (-0.002%) from 92.091%
when pulling e5ead5f on Rohde-Schwarz:span/block_cipher_internals
into 38a0b56 on randombit:master.

@reneme reneme force-pushed the span/block_cipher_internals branch from 927ac16 to d7605c2 Compare January 4, 2024 07:46
@reneme reneme force-pushed the span/block_cipher_internals branch 2 times, most recently from 506ea76 to 7c61322 Compare March 25, 2024 10:54
This demotes Block_Cipher::encrypt_n/decrypt_n to top-level methods
and introduces new (private) virtual methods (encrypt/decrypt_blocks)
that use std::span for the in/out buffers. Also, this adapts all block
cipher implementations in the library to use the new API.
@randombit
Copy link
Owner

Sorry just getting back to this.

I'm not sold on this change as it stands because it has two effects

  1. We are now accepting arguments from the user, and ... just not checking then. An implicit assumption of the interface (with or without span) is that both arrays point to the same length of memory, namely blocks*BLOCK_SIZE bytes, and also that they don't overlap. However now (with span) we are allowing the user to tell us via the span lengths "oh no these input and output arrays aren't the same length, lol lmao" but we blithely ignore it. Likewise we still require the user to tell us the number of blocks, even though we can deduce it from the span lengths.

  2. Overall the span based flow is harder to understand (at least for me) than ptr + index.

That said span does have potential to improve the internal implementations of the block ciphers (for example #3942) However I think a better approach here is instead of a wholesale conversion of the ciphers is to instead pick a single one [*] and use it as a testbed for experimentation of approaches to ease implementation. Effectively

void encrypt_n(const uint8_t inb[], uint8_t outb[], size_t blocks) {
   std::span<const uint8_t> in(inb, inb + blocks*block_size());
   std::span<uint8_t> out(outb, outb + blocks*block_size());

   // span-oriented cipher follows
}

Then when things reach the point that it's clear there are substantial improvements in usability we can switch over wholesale.

[*] I've traditionally used Blowfish as my arbitrary choice for these kinds of things, because it's very fast so any performance impacts are visible, and it's not "important" so if bugs are introduced it's not the end of the world.

@reneme reneme modified the milestones: Botan 3.4.0, Botan 3.5.0 Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants