code review 1870047: asn1 incorrectly encoding integers. When determining the

asn1 incorrectly encoding integers. When determining the encoded length it is not taking into account the sign bit. Fixes issue 997.

[nwaples, 2010-08-06 01:42:00]

Hello agl@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change.

[agl1, 2010-08-06 15:23:22]

LGTM. Will land once I ask rsc what the deal with the CONTRIBUTORS file is these days.

[gri, 2010-08-06 16:35:05]

http://codereview.appspot.com/1870047/diff/2001/3001 File src/pkg/asn1/marshal.go (right): http://codereview.appspot.com/1870047/diff/2001/3001#newcode128 src/pkg/asn1/marshal.go:128: if i > 0 { This if is not needed. If i <= 0, the for loop won't execute with or w/o the if and there's nothing else that happens if i > 0. After the for loop ended, i will be <= 127, but it will still be >= 0. http://codereview.appspot.com/1870047/diff/2001/3001#newcode135 src/pkg/asn1/marshal.go:135: if i < 0 { Same applies here: this if is not needed. If i is >= 0 the for loop won't execute. i will never be < 0 even if the previous for loop executed.

[nwaples, 2010-08-07 00:20:30]

Hello agl1, gri (cc: golang-dev@googlegroups.com), I'd like you to review this change.

[agl1, 2010-08-09 14:26:08]

*** Submitted as http://code.google.com/p/go/source/detail?r=123cc282ec65 *** asn1 incorrectly encoded signed integers. When determining the encoded length it was not taking into account the sign bit. Fixes issue 997. R=agl1, gri CC=golang-dev http://codereview.appspot.com/1870047 Committer: Adam Langley <agl@golang.org>

[chressie, 2010-08-09 15:03:00]

On Mon, Aug 9, 2010 at 4:26 PM, <agl@golang.org> wrote: > *** Submitted as > http://code.google.com/p/go/source/detail?r=123cc282ec65 *** It seems the submitted CL[1] is not the latest CL[2]. The submitted CL still includes the unnecessary if-statements before the for-loops. Kind regards, chressie [1]: <http://code.google.com/p/go/source/detail?r=123cc282ec65> [2]: <http://codereview.appspot.com/1870047/diff/11001/12001> 

[gri1, 2010-08-10 17:20:12]

This has been fixed in the meantiome: https://code.google.com/p/go/source/detail?r=16757c23fc3b715d75d34f25c72a16c4... Please abandon. Thanks. - gri On Thu, Aug 5, 2010 at 6:42 PM, <nwaples@gmail.com> wrote: > Reviewers: agl1, > > Message: > Hello agl@golang.org (cc: golang-dev@googlegroups.com), > > > I'd like you to review this change. > > > Description: > asn1 incorrectly encoding integers. When determining the > encoded length it is not taking into account the sign bit. > Fixes issue 997. > > Please review this at http://codereview.appspot.com/1870047/show > > Affected files: > > M src/pkg/asn1/marshal.go > M src/pkg/asn1/marshal_test.go > > > Index: src/pkg/asn1/marshal.go > =================================================================== > --- a/src/pkg/asn1/marshal.go > +++ b/src/pkg/asn1/marshal.go > @@ -123,13 +123,20 @@ > } > > func int64Length(i int64) (numBytes int) { > - if i == 0 { > - return 1 > + numBytes = 1 > + > + if i > 0 { > + for i > 127 { > + numBytes++ > + i >>= 8 > + } > } > > - for i > 0 { > - numBytes++ > - i >>= 8 > + if i < 0 { > + for i < -128 { > + numBytes++ > + i >>= 8 > + } > } > > return > Index: src/pkg/asn1/marshal_test.go > =================================================================== > --- a/src/pkg/asn1/marshal_test.go > +++ b/src/pkg/asn1/marshal_test.go > @@ -59,6 +59,10 @@ > > var marshalTests = []marshalTest{ > marshalTest{10, "02010a"}, > + marshalTest{127, "02017f"}, > + marshalTest{128, "02020080"}, > + marshalTest{-128, "020180"}, > + marshalTest{-129, "0202ff7f"}, > marshalTest{intStruct{64}, "3003020140"}, > marshalTest{twoIntStruct{64, 65}, "3006020140020141"}, > marshalTest{nestedStruct{intStruct{127}}, "3005300302017f"}, > > >