code review 8303043: crypto/rand: better panic message for invalid Int argument.

crypto/rand: better panic message for invalid Int argument.

Also document the panic to be consistent with math/rand.

Fixes issue 5187.

[remyoudompheng, 2013-04-03 06:15:41]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2013-04-03 06:18:21]

Thank you. Leaving for the release candidate masters to decide if it makes the
cut.

https://codereview.appspot.com/8303043/diff/5001/src/pkg/crypto/rand/util.go
File src/pkg/crypto/rand/util.go (right):

https://codereview.appspot.com/8303043/diff/5001/src/pkg/crypto/rand/util.go#...
src/pkg/crypto/rand/util.go:106: panic("crypto/rand: argument to Int is <= 0")
Could you change math/rand at the same time to use this wording, I think it is
an improvement over what exists now.

[bradfitz, 2013-04-03 07:50:14]

LGTM

Don't care about nil too?



On Tue, Apr 2, 2013 at 11:15 PM, <remyoudompheng@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> crypto/rand: better panic message for invalid Int argument.
>
> Also document the panic to be consistent with math/rand.
>
> Fixes issue 5187.
>
> Please review this at
https://codereview.appspot.**com/8303043/<https://codereview.appspot.com/8303...
>
> Affected files:
>   M src/pkg/crypto/rand/util.go
>
>
> Index: src/pkg/crypto/rand/util.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/rand/util.go
> +++ b/src/pkg/crypto/rand/util.go
> @@ -100,8 +100,11 @@
>         }
>  }
>
> -// Int returns a uniform random value in [0, max).
> +// Int returns a uniform random value in [0, max). It panics if max <= 0.
>  func Int(rand io.Reader, max *big.Int) (n *big.Int, err error) {
> +       if max.Sign() <= 0 {
> +               panic("crypto/rand: argument to Int is <= 0")
> +       }
>         k := (max.BitLen() + 7) / 8
>
>         // b is the number of bits in the most significant byte of max.
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[remyoudompheng, 2013-04-08 07:05:54]

Is this okay for Go 1.1?

What about:
- the rand package?
- the nil pointer case?

Do you advise something?

[bradfitz, 2013-04-08 07:09:06]

Seems fine for 1.1, but I'm not concerned either way.

If it's going to crash either way, we can crash with a nice error message.
Or not.

Likewise with nil.  Your call.






On Mon, Apr 8, 2013 at 12:05 AM, <remyoudompheng@gmail.com> wrote:

> Is this okay for Go 1.1?
>
> What about:
> - the rand package?
> - the nil pointer case?
>
> Do you advise something?
>
>
https://codereview.appspot.**com/8303043/<https://codereview.appspot.com/8303...
>

[r, 2013-04-15 19:53:47]

LGTM

[bradfitz, 2013-04-18 06:21:19]

*** Submitted as https://code.google.com/p/go/source/detail?r=4543c39256f9 ***

crypto/rand: better panic message for invalid Int argument.

Also document the panic to be consistent with math/rand.

Fixes issue 5187.

R=golang-dev, dave, bradfitz, r
CC=golang-dev
https://codereview.appspot.com/8303043

Committer: Brad Fitzpatrick <bradfitz@golang.org>