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

cipher: revert SeekNum::from_block_byte change #439

Merged
merged 1 commit into from Dec 30, 2020

Conversation

tarcieri
Copy link
Member

This reverts the implementation of SeekNum::from_block_byte which was merged as part of #435.

I'm not exactly sure what the issue is and it's somewhat difficult to debug in that it's code that involves both generics and macros causing an error as part of any failure in a long chain of checked arithmetic, which I'm trying to debug from the context of a concrete stream cipher impl (both chacha20 and salsa20) where it's operating over a generic type.

The error manifests as OverflowError:

RustCrypto/stream-ciphers#205 (comment)

This commit reverts to the previous implementation, which is at least much simpler.

This reverts the implementation of `SeekNum::from_block_byte` which was
merged as part of #435.

I'm not exactly sure what the issue is and it's somewhat difficult to
debug in that it's code that involves both generics and macros causing
an error as part of any failure in a long chain of checked arithmetic,
which I'm trying to debug from the context of a concrete stream cipher
impl (both `chacha20` and `salsa20`) where it's operating over a generic
type.

The error manifests as `OverflowError`:

RustCrypto/stream-ciphers#205 (comment)

This commit reverts to the previous implementation, which is at least
much simpler.
@tarcieri
Copy link
Member Author

I've confirmed this fixes the test failures in the chacha20 and salsa20 crates locally.

@tarcieri tarcieri merged commit 2f6287c into master Dec 30, 2020
@tarcieri tarcieri deleted the cipher/revert-seeknum-changes branch December 30, 2020 04:09
@tarcieri
Copy link
Member Author

@newpavlov this appears to be a bug in the new implementation of this logic, although I'm not sure what specifically the problem was

@newpavlov
Copy link
Member

I will try to look into it.

@newpavlov
Copy link
Member

Ah, I've changed how block-buffer handles counters to simplify downstream implementations. Let's say we have two counters (block_counter, buffer_counter), assuming we process one byte at a time previously we had sequence (0, 0), (0, 1), (0, 2), etc. In the new version we have the following sequence instead: (0, 0), (1, 1), (1, 2), etc. It allows to merge block counter increment and generation of a new block.

@tarcieri
Copy link
Member Author

@newpavlov is there something that needs to change in chacha20 and salsa20? I was confused why their test suites were failing with OverflowError:

RustCrypto/stream-ciphers#205 (comment)

@newpavlov
Copy link
Member

Probably, yes. I will need some time to refresh my memory of how they structured. When working on the cipher PR i was mostly working with the ctr crate, so I am not sure how compatible the introduced changes with chacha20 and other stream ciphers.

@tarcieri
Copy link
Member Author

tarcieri commented Dec 30, 2020

Both chacha20 and salsa20 contain what was originally copypasta from the ctr crate, so it likely needs to be updated in both.

Hopefully they can lean more heavily on block-buffer going forward to simplify these updates.

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

2 participants