Skip to content

Conversation

@newpavlov
Copy link
Member

Closes #566

Copy link
Contributor

@jonasmalacofilho jonasmalacofilho left a comment

Choose a reason for hiding this comment

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

Note that my allocation-only benchmark in #566 suggests this could be a tiny bit slower than vec![Block::default, ...].

So maybe it would be good to benchmark this with high-ish memory sizes, or at least with the 19 MiB and 46 MiB sizes recommended by OWASP?

@tarcieri
Copy link
Member

tarcieri commented Mar 4, 2025

Maybe we should try to land #547 first so we can gauge any impact on performance?

@newpavlov
Copy link
Member Author

newpavlov commented Mar 5, 2025

Maybe we should try to land #547 first so we can gauge any impact on performance?

I think we should merge this PR, then #547 without the alignment workaround after it was migrated to Blocks, and then finally add the alignment workaround to Blocks in a separate PR. I am still not 100% convinced that we should add the workaround. It needs additional investigation in my opinion.

@newpavlov newpavlov merged commit 6679cbd into master Mar 5, 2025
18 checks passed
@newpavlov newpavlov deleted the argon2/alloc_zeroed branch March 5, 2025 13:14
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.

Argon2::hash_password_into should use fallible memory allocations

4 participants