Hacker News new | ask | show | jobs
by amluto 342 days ago
On an extremely quick review:

- This uses global state under the hood. Surprise! Is it thread safe? I’m not a Go expert, but it looks non-thread-safe.

- The copying code reminds me of old-school awful C buffer handling code. Maybe it’s right. Maybe it’s wrong. But it’s not obviously right.

- The actual meat is a cryptographic randomness cache. This is a subtle thing, and all the best practices are missing. Where’s the backtracking protection? What if the program forks? vDSO getrandom() knows how to do this correctly — something high-level should use it, not reimplement it incorrectly.

3 comments

Global use looks fine - it's a very-simply-used sync pool to do larger blocks of rand reads, which makes plenty of sense for performance.

Unsafe use also looks fine, values either don't escape the function (a type string->byte type cast for function signature reasons) or they do but they're new temporary data (the byte->string cast, which is fine because there's no risk of reusing or modifying the original bytes).

I'm going to intentionally not make any claims to "cryptographic security" or "is this a GUID" as I'm not super clear on the details there. The code looks pretty normal to me though, with the possible exception of the base64 encoding (why not base64.URLEncoding? https://pkg.go.dev/encoding/base64#pkg-variables).

> This uses global state under the hood.

Looks safe to me. It uses `crypto/rand.Read` which is declared as safe for concurrent use. The cache is accessed via sync.Pool which is thread safe. As a check, I ran the tests with `-race` and it passed.

Thanks for your feedback. If you are skilled in Golang, I suggest you review the code more thoroughly for a more accurate understanding (especially compared to what standard uuid does).