Skip to content

use shared buffer for retrieving random bytes #5444

Merged
anonrig merged 3 commits intomainfrom
yagiz/buffered-entropy-source
Nov 6, 2025
Merged

use shared buffer for retrieving random bytes #5444
anonrig merged 3 commits intomainfrom
yagiz/buffered-entropy-source

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Oct 30, 2025

Another take to #5442 which should be a lot faster since we're basically caching now.

@anonrig anonrig requested a review from kentonv October 30, 2025 15:51
@anonrig anonrig requested review from a team as code owners October 30, 2025 15:51
@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch 2 times, most recently from 0c342d0 to 8d2b2b6 Compare October 30, 2025 16:14
@jasnell
Copy link
Collaborator

jasnell commented Oct 30, 2025

Needs tests for the new utility

@anonrig anonrig requested a review from jasnell October 30, 2025 16:53
@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch 2 times, most recently from 0aeedfc to cd9aa70 Compare October 30, 2025 17:03
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 30, 2025

CodSpeed Performance Report

Merging #5444 will not alter performance

Comparing yagiz/buffered-entropy-source (f69afd4) with main (27391e9)

Summary

✅ 34 untouched
⏩ 9 skipped1

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch from cd9aa70 to d8f1256 Compare October 30, 2025 17:57
@kentonv
Copy link
Member

kentonv commented Oct 30, 2025

This design based on EntropySource isn't what I had in mind. I think we should define a simple global function like:

void getEntropy(kj::ArrayPtr<byte> output);

This should use a thread_local buffer in its implementation.

Then we can replace all the RAND_bytes calls with this.

This makes the change light-touch with no need to change any design around EntropySource.

@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch from d8f1256 to 4642d32 Compare October 30, 2025 20:59
@anonrig anonrig requested a review from jasnell October 30, 2025 21:00
Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

OK, this now looks pretty good to me but I wonder if we should check in with a security person to be extra-sure we aren't doing something bad here -- random number generation problems can be catastrophic for crypto.

@mschwarzl what do you think?

@anonrig anonrig changed the title implement BufferedEntrophySource to reduce syscalls use shared buffer for retrieving random bytes Oct 30, 2025
@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch from 4642d32 to 63ec89a Compare October 31, 2025 13:57
@anonrig anonrig requested review from jasnell and kentonv October 31, 2025 13:57
@jasnell jasnell requested a review from mschwarzl October 31, 2025 19:46
@jasnell
Copy link
Collaborator

jasnell commented Nov 4, 2025

Point raised from the security team: is forking an issue here? Specifically, if the process is forked after the cache is populated, the cache for the current thread will be duplicated in the fork. We either need to (a) require that the cache is only populated after forking happens or (b) refresh the cache when forking happens.

@jclee
Copy link
Contributor

jclee commented Nov 4, 2025

Point raised from the security team: is forking an issue here? Specifically, if the process is forked after the cache is populated, the cache for the current thread will be duplicated in the fork. We either need to (a) require that the cache is only populated after forking happens or (b) refresh the cache when forking happens.

FWIW, since this code is most likely invoked in the sandbox, it shouldn't be forking. We avoid forking in a process that already has additional threads, since another thread could be holding locks that would never release in the new process. In the past, this has typically manifested as "interrupt" signals in tcmalloc functions.

@kentonv
Copy link
Member

kentonv commented Nov 4, 2025

This code should be called strictly post-fork, but we should nevertheless add some debug checks to verify that the value returned by getpid() hasn't changed -- and just crash if a change is seen. If we see such a crash in tests, it means there's some pre-fork call to getEntropy() that needs to be removed.

We should skip these checks in release builds as getpid() is a syscall which is expensive.

@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch from 63ec89a to f7310ec Compare November 5, 2025 21:48
@anonrig
Copy link
Member Author

anonrig commented Nov 5, 2025

Applied the pid check, rebased and force pushed.

@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch from f7310ec to 0d8310f Compare November 6, 2025 02:34
@anonrig anonrig force-pushed the yagiz/buffered-entropy-source branch from 0d8310f to f69afd4 Compare November 6, 2025 02:34
@anonrig anonrig merged commit 14444e1 into main Nov 6, 2025
43 of 45 checks passed
@anonrig anonrig deleted the yagiz/buffered-entropy-source branch November 6, 2025 14:28
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.

4 participants