Skip to content

Reject zero r or p in reference scrypt#406

Open
achimala wants to merge 1 commit into
Tarsnap:masterfrom
achimala:codex/ref-param-rp-zero
Open

Reject zero r or p in reference scrypt#406
achimala wants to merge 1 commit into
Tarsnap:masterfrom
achimala:codex/ref-param-rp-zero

Conversation

@achimala

Copy link
Copy Markdown
Contributor

Summary

  • reject r == 0 or p == 0 in crypto_scrypt() with EINVAL
  • document the lower bound as 0 < r * p < 2^30 in FORMAT, README.md, scrypt-kdf.h, and the reference implementation comment

Notes

This follows up on 686a628 as requested in #405.

Tests

  • git diff --check

Split out from #405.

@gperciva

Copy link
Copy Markdown
Member

Thanks, the code looks great!

Two nitpicks in the commit message, one of them my fault:

  1. please wrap text before 80 chars. 78 or 76 would be nice.
  2. I gave you the hash 686a628 for the commit as it appeared in PR Don't allow r or p to be zero #177, but the hash changed when it was merged (or rebased?) into master. The actual hash in master is d223018; please update this. I apologies for my mistake.

The file format and libscrypt-kdf API require a positive r * p
product, but crypto_scrypt() did not reject zero r or p before later
size checks.

Reject zero values with EINVAL and document the positive lower bound
explicitly.

This follows up on d223018.
@achimala achimala force-pushed the codex/ref-param-rp-zero branch from 2a15ba1 to 333de9e Compare May 31, 2026 01:45
@achimala

Copy link
Copy Markdown
Contributor Author

Thanks, updated. I amended the commit message to wrap before 80 chars and fixed the historical reference to use the merged master commit d223018.

@gperciva

gperciva commented Jul 8, 2026

Copy link
Copy Markdown
Member

Closing & reopening to trigger a CI run.

@gperciva gperciva closed this Jul 8, 2026
@gperciva gperciva reopened this Jul 8, 2026
@cperciva

cperciva commented Jul 8, 2026

Copy link
Copy Markdown
Member

Looks like this collided with #407; can you fix it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants