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

Jump to bottom

crypto/rand: Reader is internally buffered inconsistently on different platforms #16593

Closed
takeyourhatoff opened this issue Aug 4, 2016 · 10 comments
Closed

crypto/rand: Reader is internally buffered inconsistently on different platforms #16593

takeyourhatoff opened this issue Aug 4, 2016 · 10 comments
Assignees
@FiloSottile
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Milestone
Backlog

Comments

@takeyourhatoff
Copy link

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    1.6.3
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="linux"
    GOOS="linux"
    GOPATH="/home/tyho/go"
    GORACE=""
    GOROOT="/usr/lib/golang"
    GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
    GO15VENDOREXPERIMENT="1"
    CC="gcc"
    GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
    CXX="g++"
    CGO_ENABLED="1"
  3. What did you expect to see?
    rand.Reader to consistently internally buffer, or not internally buffer calls to Read regardless of platform.
  4. What did you see instead?
    On generic Unix, including old versions of Linux (without a getrandom syscall), rand.Reader reads from /dev/urandom, and buffers calls to Read using the bufio package. On modern Linux however, rand.Read requests are implemented using the new getrandom syscall, but the calls are not buffered.
  5. What did you do?
    I tested to see if internally buffering output on platforms with the getrandom syscall available would make a significant difference to the performance of small 16B reads:

https://play.golang.org/p/80fyLlBFQb

On my machine, internally buffering the output of Read doubled performance compared with raw Reads. Tv` suggested that the unbuffered Reads may be quicker when multithreaded due to bufio adding a mutex. He wrote a benchmark to test this theory but it turned out not to be true:

https://play.golang.org/p/3sO47iNhK5
@ianlancetaylor
Copy link
Contributor

Dup of #16124 ?

@josharian
Copy link
Contributor

Not a dup, I think. That was math/rand; this is crypto/rand.

@bradfitz bradfitz changed the title crypto/rand: Reader is internally buffered inconsistantly on different platforms crypto/rand: Reader is internally buffered inconsistently on different platforms Aug 4, 2016
@bradfitz bradfitz added this to the Unplanned milestone Aug 4, 2016
@agl agl self-assigned this Aug 19, 2016
@agl
Copy link
Contributor

agl commented Sep 30, 2016

Not buffering is actually a nice feature of the getrandom code. Although Go programs don't fork, thus eliminating one of the classic sources of problems, these days there's hibernation, VM migrations etc.

What's the situation where the syscall performance is a problem? (Is it CBC-mode in TLS again?)

@takeyourhatoff takeyourhatoff mentioned this issue Jul 25, 2019
Closed
@CAFxX
Copy link
Contributor

CAFxX commented Aug 2, 2019 •
edited

Still applies to 1.12 and tip. See #33256 for benchmarks showing order-of-magnitude differences for small reads via a bufio.Reader.

Although Go programs don't fork, thus eliminating one of the classic sources of problems, these days there's hibernation, VM migrations etc.

Don't these apply equally to the entropy pool in the kernel? Why is it a concern only for entropy buffered in process? (Also note that the /dev/urandom fallback already buffers.)

What's the situation where the syscall performance is a problem?

We ran into this in the profile of a workload where we need to generate tokens for authentication (unrelated to TLS).

@gopherbot
Copy link

Change https://golang.org/cl/331269 mentions this issue: crypto/rand: buffer the entropy obtained by calling unix.GetRandom

@magical
Copy link
Contributor

magical commented Jun 28, 2021

I'd like to mildly object to CL 331269. Not buffering has nice security properties that agl mentioned above, getentropy should be fairly fast, and in cases where the syscall overhead is actually noticable it should be simple for the user to wrap crypto/rand.Reader in a bufio.Reader (which is essentially what the CL does). The user is in a better position to know when buffers can be shared, too. By introducing a global buffer, the CL introduces contention where there needn't be any.

@andrewchambers
Copy link

I think its better to never buffer it, then clients can decide to buffer it without accidentally double buffering on some platforms.

@FiloSottile FiloSottile assigned FiloSottile and unassigned agl Mar 2, 2022
@FiloSottile FiloSottile modified the milestones: Unplanned, Go1.19 Mar 2, 2022
@FiloSottile FiloSottile added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed help wanted labels Mar 2, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/security

Doesn't look like this is happening for 1.19. Moving milestone to backlog. Please recategorize if appropriate.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Backlog Jun 24, 2022
@magical
Copy link
Contributor

magical commented Jun 24, 2022

I think this issue is fixed/superseded by the pair of commits:

  • d68a8d0 (which added buffering for getentropy/getrandom)
  • 3ae414c (which removed all buffering)

@rolandshoemaker
Copy link
Member

Yes, I believe this was fixed (and is included in 1.19.)

@golang golang locked and limited conversation to collaborators Jun 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@FiloSottile FiloSottile

Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Projects
Release Status & Release Blockers
Status: Done
Milestone
Backlog
Development

No branches or pull requests