Skip to content

chacha20: fix big endian failure and test in CI#447

Merged
tarcieri merged 4 commits intoRustCrypto:masterfrom
nstilt1:big-endian-testing
Aug 23, 2025
Merged

chacha20: fix big endian failure and test in CI#447
tarcieri merged 4 commits intoRustCrypto:masterfrom
nstilt1:big-endian-testing

Conversation

@nstilt1
Copy link
Contributor

@nstilt1 nstilt1 commented Aug 23, 2025

Big endian tests don't currently run tests in src/rng.rs. If they did they would fail. Working on getting this yml thing working.

@nstilt1
Copy link
Contributor Author

nstilt1 commented Aug 23, 2025

Not sure why it's only running fmt and clippy

@tarcieri
Copy link
Member

Not sure why it's only running fmt and clippy

@nstilt1 yeah there's some pre-existing misconfiguration, it's missing the CI config in its triggers:

on:
  pull_request:
    paths:
      - ".github/workflows/chacha20.yml"
      - "chacha20/**"
      - "Cargo.*"
  push:
    branches: master
    paths:
      - ".github/workflows/chacha20.yml"
      - "chacha20/**"
      - "Cargo.*"

@nstilt1
Copy link
Contributor Author

nstilt1 commented Aug 23, 2025

There we go, there are the failing tests. Do you want me to move all of the rng.rs tests to /tests now? I think it should wait until after the #439 passes because I might have added a test to rng.rs and I don't want to have to move tests in 2 PRs when I could do it in 1. It doesn't have to be done in #439 but I don't want it to be before #439.

@nstilt1 yeah there's some pre-existing misconfiguration, it's missing the CI config in its triggers:

I'm not sure what you want me to do with that... I'm new to this CI/CD stuff.

@nstilt1
Copy link
Contributor Author

nstilt1 commented Aug 23, 2025

Or should I add that big endian cfg back in this PR?

@tarcieri
Copy link
Member

#448 fixes the CI misconfiguration I noted earlier.

Thanks for reproducing the big endian failure. Now that we know it's broken if you can fix it in this same PR that'd be great, even if the fix is just changing it back to what it was before.

@tarcieri tarcieri changed the title chacha20: revised big endian tests to use all features chacha20: fix big endian failure and test in CI Aug 23, 2025
@tarcieri tarcieri merged commit b247cd5 into RustCrypto:master Aug 23, 2025
27 checks passed
Comment on lines +365 to +368
#[cfg(target_endian = "big")]
for word in r.0.iter_mut() {
*word = word.to_le();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is wrong: there is no type change here (u32 to u32) so there should be no endian-switch.

We also don't do this in rand_chacha.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug through rand_chacha and ppv-lite86 for a few minutes and couldn't find anywhere with endian conversions regarding the output. But multiple tests fail on big endian. If you look closely, you can see that the bytes within the words are in reverse order. I don't know why they are in reverse order, but the tests pass as soon as to_le() is called on all of the words.

https://github.com/RustCrypto/stream-ciphers/actions/runs/17169966574/job/48717538576

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have found the problem. Line 48 of soft.rs converts each word to little endian. Big endian code will most likely be hitting the soft.rs backend. Even if this isn't the culprit, then this part of the code should be using conditional compilation. Will change the code and run a test right quick

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep... that was it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm pretty sure the backends shouldn't be doing any endian conversions.

nstilt1 added a commit to nstilt1/stream-ciphers-rng that referenced this pull request Aug 26, 2025
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.

3 participants