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

crypto/rand: Read argument escapes to heap #66779

Closed
FiloSottile opened this issue Apr 11, 2024 · 6 comments
Closed

crypto/rand: Read argument escapes to heap #66779

FiloSottile opened this issue Apr 11, 2024 · 6 comments
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted.
Milestone

Comments

@FiloSottile
Copy link
Contributor

Currently, Read is implemented as io.ReadFull(Reader, b) which escape analysis can't track, so b escapes to the heap.

This is somewhat annoying, since it causes allocations in paths that could otherwise have none.

It should be possible to rewrite the package so that it doesn't escape on major platforms.

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 11, 2024
@FiloSottile FiloSottile added this to the Backlog milestone Apr 11, 2024
@mateusz834
Copy link
Member

I' ve been investigating this once, and the remaining problem was that the rand.Reader is an interface and cases like:

var Reader io.Reader = &someStruct{}

will always cause the buffer passed to rand.Reader.Read to be escaped.

@mateusz834
Copy link
Member

But i guess we can make the rand.Read function escape-free.

@mateusz834 mateusz834 self-assigned this Apr 12, 2024
@mateusz834 mateusz834 added FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Apr 12, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/578516 mentions this issue: crypto/rand: make Read not escape the byte slice

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/602498 mentions this issue: crypto/rand: prevent Read argument from escaping to heap

mateusz834 added a commit to mateusz834/go that referenced this issue Sep 30, 2024
Updates golang#66779

Change-Id: Iaa3eda4309bb1e8c28136dd048479e7269f4c189
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616696 mentions this issue: runtime, internal/syscall/unix: mark getrandom vDSO as non-escaping

gopherbot pushed a commit that referenced this issue Sep 30, 2024
Updates #66779
Updates #69577

Change-Id: I0dea5a30aab87aaa443e7e6646c1d07aa865ac1c
GitHub-Last-Rev: 1cea46d
GitHub-Pull-Request: #69719
Reviewed-on: https://go-review.googlesource.com/c/go/+/616696
LUCI-TryBot-Result: Go LUCI <[email protected]>
Commit-Queue: Ian Lance Taylor <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/618275 mentions this issue: crypto/rand: skip TestAllocations if optimizations are off

@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Oct 7, 2024
gopherbot pushed a commit that referenced this issue Oct 7, 2024
Without optimizations escape analysis can't do as much.

Updates #66779

Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-noopt
Change-Id: I9ccd1b995c62427ceebd9ce5c98170dbf4a93e8b
Reviewed-on: https://go-review.googlesource.com/c/go/+/618275
Reviewed-by: Roland Shoemaker <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted.
Projects
None yet
Development

No branches or pull requests

4 participants