code review 5437081: math/big: fix destination leak into result value

math/big: fix destination leak into result value
This code would panic:
z := big.NewInt(1)
z.SetBit(big.NewInt(0), 2, 1)
if z.Cmp(big.NewInt(1<<2)) != 0 {
        panic("fail")
}

[rog, 2011-11-29 18:11:28]

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

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

[gri, 2011-11-29 18:31:04]

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/int_test.go
File src/pkg/math/big/int_test.go (right):

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/int_test.go#...
src/pkg/math/big/int_test.go:1251: t.Errorf("destination value not leaked into
result value; got %s want 4", z)
this is a confusing message - if the test fails it means that the old
destination value (z) actually leaked into the result - so it should be "leaked"
instead of "not leaked". perhaps just leave away the part before the ;

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/nat.go
File src/pkg/math/big/nat.go (right):

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/nat.go#newco...
src/pkg/math/big/nat.go:923: copy(z, x)
you can still factor out copy after the if statement

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/nat.go#newco...
src/pkg/math/big/nat.go:924: z[len(x):].clear()
s/len(x)/n/

for consistency

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/nat.go#newco...
src/pkg/math/big/nat.go:927: copy(z, x)
remove

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/nat.go#newco...
src/pkg/math/big/nat.go:928: }
copy(z, x)

[rog, 2011-11-30 08:55:46]

PTAL

(also made a minor change to make a test print consistent)

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/int_test.go
File src/pkg/math/big/int_test.go (right):

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/int_test.go#...
src/pkg/math/big/int_test.go:1251: t.Errorf("destination value not leaked into
result value; got %s want 4", z)
On 2011/11/29 18:31:04, gri wrote:
> this is a confusing message - if the test fails it means that the old
> destination value (z) actually leaked into the result - so it should be
"leaked"
> instead of "not leaked". perhaps just leave away the part before the ;

yes, the "not" is spurious. i did this in too much of a hurry.

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/nat.go
File src/pkg/math/big/nat.go (right):

http://codereview.appspot.com/5437081/diff/1002/src/pkg/math/big/nat.go#newco...
src/pkg/math/big/nat.go:923: copy(z, x)
On 2011/11/29 18:31:04, gri wrote:
> you can still factor out copy after the if statement

good point.
done.

[gri, 2011-11-30 17:29:14]

LGTM
thx for fixing this!
- gri

[gri, 2011-11-30 17:30:02]

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

math/big: fix destination leak into result value
This code would panic:
z := big.NewInt(1)
z.SetBit(big.NewInt(0), 2, 1)
if z.Cmp(big.NewInt(1<<2)) != 0 {
        panic("fail")
}

R=rsc, gri
CC=golang-dev
http://codereview.appspot.com/5437081

Committer: Robert Griesemer <gri@golang.org>