code review 1773041: crypto/rand for Windows

crypto/rand for Windows

[peterGo, 2010-07-09 02:22:26]

Hello rsc, brainman (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[peterGo, 2010-07-09 04:46:52]

Hello rsc, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-07-09 05:00:40]

One more brick in the wall <g>. Thank you.

http://codereview.appspot.com/1773041/diff/7001/8004
File src/pkg/crypto/rand/rand_windows.go (right):

http://codereview.appspot.com/1773041/diff/7001/8004#newcode26
src/pkg/crypto/rand/rand_windows.go:26: if r.prov == 0 {
I would use once.Do(...) here instead.

[peterGo, 2010-07-09 16:49:24]

These are cosmetic changes, primarily improved documentation.

http://codereview.appspot.com/1773041/diff/7001/8003
File src/pkg/crypto/rand/rand_unix.go (right):

http://codereview.appspot.com/1773041/diff/7001/8003#newcode5
src/pkg/crypto/rand/rand_unix.go:5: // Package rand implements a
cryptographically secure
Redundant; we already said this in rand.go.

http://codereview.appspot.com/1773041/diff/7001/8004
File src/pkg/crypto/rand/rand_windows.go (right):

http://codereview.appspot.com/1773041/diff/7001/8004#newcode5
src/pkg/crypto/rand/rand_windows.go:5: // Package rand implements a
cryptographically secure
Redundant; we already said this in rand.go.

http://codereview.appspot.com/1773041/diff/7001/8004#newcode18
src/pkg/crypto/rand/rand_windows.go:18: 
There's an implementation comment for Unix.

http://codereview.appspot.com/1773041/diff/7001/8004#newcode19
src/pkg/crypto/rand/rand_windows.go:19: type rngReader struct {
This rates a comment in Unix.

http://codereview.appspot.com/1773041/diff/7001/8004#newcode42
src/pkg/crypto/rand/rand_windows.go:42: func init() { Reader = &rngReader{} }
The init() function should be in the same location in the source code for both
Unix and Windows.

[peterGo, 2010-07-09 16:58:30]

Hello rsc, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[peterGo, 2010-07-10 02:43:00]

http://codereview.appspot.com/1773041/diff/7001/8004
File src/pkg/crypto/rand/rand_windows.go (right):

http://codereview.appspot.com/1773041/diff/7001/8004#newcode26
src/pkg/crypto/rand/rand_windows.go:26: if r.prov == 0 {
On 2010/07/09 05:00:42, brainman wrote:
> I would use once.Do(...) here instead.

By design, since they are solving the essentially same problem, the Unix and
Windows Read methods use the same pattern. If you want to improve the Windows
Read method by using once.Do(...), shouldn't you make the same improvement to
the Unix Read method?

Even if we do use once.Do(...), I would still test for nil or zero. If a pointer
might be nil, now or in the future, I always use a guard to avoid panics; the
same with zero value handles. The file could have been closed or the handle
could have been released.

I included rsc in the list of reviewers because he's the original author of the
Unix-like crypto/rand version.

Issue  569044: code review 569044: crypto/rand: new package
http://codereview.appspot.com/569044

func (r *devReader) Read(b []byte) (n int, err os.Error)
http://codereview.appspot.com/569044/diff/17001/18003#newcode36

[peterGo, 2010-07-10 02:43:43]

Hello rsc, brainman (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[brainman, 2010-07-11 02:21:39]

> Even if we do use once.Do(...), I would still test for nil or zero. If a
pointer
> might be nil, now or in the future, I always use a guard to avoid panics; the
> same with zero value handles. 

once.Do(...) would take care of everything, that is the sole point of it.

... The file could have been closed or the handle
> could have been released.

Not in the latest version of you code.

> I included rsc in the list of reviewers ...

I'm not fussy either way.

LGTM.

[rsc1, 2010-07-12 20:15:30]

looks pretty good, thanks.

http://codereview.appspot.com/1773041/diff/16001/17002
File src/pkg/crypto/rand/rand.go (right):

http://codereview.appspot.com/1773041/diff/16001/17002#newcode16
src/pkg/crypto/rand/rand.go:16: // On Unix-like systems, Reader is initialized
to use the
..., Reader reads from /dev/urandom.

http://codereview.appspot.com/1773041/diff/16001/17002#newcode18
src/pkg/crypto/rand/rand.go:18: // On Windows systems, Reader is initialized to
use the
..., Reader uses the CryptGenRandom API.

http://codereview.appspot.com/1773041/diff/16001/17005
File src/pkg/crypto/rsa/pkcs1v15_test.go (right):

http://codereview.appspot.com/1773041/diff/16001/17005#newcode67
src/pkg/crypto/rsa/pkcs1v15_test.go:67: if random == nil {
delete

http://codereview.appspot.com/1773041/diff/16001/17005#newcode141
src/pkg/crypto/rsa/pkcs1v15_test.go:141: if random == nil {
delete

http://codereview.appspot.com/1773041/diff/16001/17006
File src/pkg/crypto/rsa/rsa_test.go (right):

http://codereview.appspot.com/1773041/diff/16001/17006#newcode17
src/pkg/crypto/rsa/rsa_test.go:17: if random == nil {
delete

http://codereview.appspot.com/1773041/diff/16001/17006#newcode80
src/pkg/crypto/rsa/rsa_test.go:80: if random == nil {
delete

http://codereview.appspot.com/1773041/diff/16001/17007
File src/pkg/crypto/x509/x509_test.go (right):

http://codereview.appspot.com/1773041/diff/16001/17007#newcode149
src/pkg/crypto/x509/x509_test.go:149: if random == nil {
delete

[peterGo, 2010-07-12 21:06:15]

Hello rsc, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[peterGo, 2010-07-12 21:06:33]

http://codereview.appspot.com/1773041/diff/16001/17002
File src/pkg/crypto/rand/rand.go (right):

http://codereview.appspot.com/1773041/diff/16001/17002#newcode16
src/pkg/crypto/rand/rand.go:16: // On Unix-like systems, Reader is initialized
to use the
On 2010/07/12 20:15:30, rsc1 wrote:
> ..., Reader reads from /dev/urandom.
> 

Done.

http://codereview.appspot.com/1773041/diff/16001/17002#newcode18
src/pkg/crypto/rand/rand.go:18: // On Windows systems, Reader is initialized to
use the
On 2010/07/12 20:15:30, rsc1 wrote:
> ..., Reader uses the CryptGenRandom API.
> 

Done.

http://codereview.appspot.com/1773041/diff/16001/17005
File src/pkg/crypto/rsa/pkcs1v15_test.go (right):

http://codereview.appspot.com/1773041/diff/16001/17005#newcode67
src/pkg/crypto/rsa/pkcs1v15_test.go:67: if random == nil {
On 2010/07/12 20:15:30, rsc1 wrote:
> delete
> 

Done.

http://codereview.appspot.com/1773041/diff/16001/17005#newcode141
src/pkg/crypto/rsa/pkcs1v15_test.go:141: if random == nil {
On 2010/07/12 20:15:30, rsc1 wrote:
> delete
> 

Done.

http://codereview.appspot.com/1773041/diff/16001/17006
File src/pkg/crypto/rsa/rsa_test.go (right):

http://codereview.appspot.com/1773041/diff/16001/17006#newcode17
src/pkg/crypto/rsa/rsa_test.go:17: if random == nil {
On 2010/07/12 20:15:30, rsc1 wrote:
> delete
> 

Done.

http://codereview.appspot.com/1773041/diff/16001/17006#newcode80
src/pkg/crypto/rsa/rsa_test.go:80: if random == nil {
On 2010/07/12 20:15:30, rsc1 wrote:
> delete
> 

Done.

http://codereview.appspot.com/1773041/diff/16001/17007
File src/pkg/crypto/x509/x509_test.go (right):

http://codereview.appspot.com/1773041/diff/16001/17007#newcode149
src/pkg/crypto/x509/x509_test.go:149: if random == nil {
On 2010/07/12 20:15:30, rsc1 wrote:
> delete
> 

Done.

[rsc, 2010-07-12 23:38:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=42e4767b10be ***

crypto/rand for Windows

R=rsc, brainman
CC=golang-dev
http://codereview.appspot.com/1773041

Committer: Russ Cox <rsc@golang.org>