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

Bugfix: Crypto ReadOnlyMemory<byte> decryption times out #1443

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

WhitWaldo
Copy link
Contributor

Description

While updating the Crypto example in preparation for creating a Quickstart, I realized that the DecryptAsync overload that accepted a ReadOnlyMemory<byte> timed out whenever it was used. I suspected the issue is related to the MemoryMarshal, which was being used to avoid an allocation. But reviewing the implementation, I realized that we were still getting that allocation later on when we created a new MemoryStream from the MemoryMarshal output.

I've instead opted to create a MemoryStream, write the input to it and reset the position to 0 to use in the EncryptAsync and DecryptAsync operations.

After making this change, both examples have been tested to work as expected (both stream and byte-based).

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • [N/A] Created/updated tests
  • Extended the documentation

…y byte arrays properly (doesn't impact stream encryption/decryption).

Signed-off-by: Whit Waldo <[email protected]>
@WhitWaldo WhitWaldo added this to the v1.15 milestone Jan 14, 2025
@WhitWaldo WhitWaldo self-assigned this Jan 14, 2025
@WhitWaldo WhitWaldo requested review from a team as code owners January 14, 2025 11:31
…mance project handles the creation of MemoryStreams without an allocation. Restored the use of MemoryMarshal, but throws an exception if the data cannot be accessed now, instead of hanging as it did in a previous iteration.

Tested both paths (string and stream) from example project successfully.

Signed-off-by: Whit Waldo <[email protected]>
philliphoff
philliphoff previously approved these changes Jan 15, 2025
Copy link
Collaborator

@philliphoff philliphoff left a comment

Choose a reason for hiding this comment

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

Just the using comment, but otherwise looks good.

Signed-off-by: Whit Waldo <[email protected]>
@WhitWaldo WhitWaldo merged commit 676c0d7 into dapr:master Jan 15, 2025
12 checks passed
@WhitWaldo WhitWaldo deleted the crypto-updates branch January 15, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants