Browse Source

cryptobyte: don't ignore bytes added to BuilderContinuations of fixed-size Builders

Builders created with NewFixedBuilder were broken when used with
BuilderContinuations. The length of the bytes written to the
continuation would get added correctly to the parent, but the actual
content would be discarded.

For example, the BytesOrPanic() in TestFixedBuilderLengthPrefixed would
return [00 08] instead of [00 08 ff ff ff ff ff ff ff ff].

Change-Id: I80837a9bf3562751addcb827274649d9f52fc79a
Reviewed-on: https://go-review.googlesource.com/c/148882
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Filippo Valsorda 7 years ago
parent
commit
3d3f9f4138
2 changed files with 33 additions and 4 deletions
  1. 6 4
      cryptobyte/builder.go
  2. 27 0
      cryptobyte/cryptobyte_test.go

+ 6 - 4
cryptobyte/builder.go

@@ -274,9 +274,11 @@ func (b *Builder) flushChild() {
 		return
 		return
 	}
 	}
 
 
-	if !b.fixedSize {
-		b.result = child.result // In case child reallocated result.
+	if b.fixedSize && &b.result[0] != &child.result[0] {
+		panic("cryptobyte: BuilderContinuation reallocated a fixed-size buffer")
 	}
 	}
+
+	b.result = child.result
 }
 }
 
 
 func (b *Builder) add(bytes ...byte) {
 func (b *Builder) add(bytes ...byte) {
@@ -284,7 +286,7 @@ func (b *Builder) add(bytes ...byte) {
 		return
 		return
 	}
 	}
 	if b.child != nil {
 	if b.child != nil {
-		panic("attempted write while child is pending")
+		panic("cryptobyte: attempted write while child is pending")
 	}
 	}
 	if len(b.result)+len(bytes) < len(bytes) {
 	if len(b.result)+len(bytes) < len(bytes) {
 		b.err = errors.New("cryptobyte: length overflow")
 		b.err = errors.New("cryptobyte: length overflow")
@@ -304,7 +306,7 @@ func (b *Builder) Unwrite(n int) {
 		return
 		return
 	}
 	}
 	if b.child != nil {
 	if b.child != nil {
-		panic("attempted unwrite while child is pending")
+		panic("cryptobyte: attempted unwrite while child is pending")
 	}
 	}
 	length := len(b.result) - b.pendingLenLen - b.offset
 	length := len(b.result) - b.pendingLenLen - b.offset
 	if length < 0 {
 	if length < 0 {

+ 27 - 0
cryptobyte/cryptobyte_test.go

@@ -412,6 +412,33 @@ func TestUnwrite(t *testing.T) {
 	})
 	})
 }
 }
 
 
+func TestFixedBuilderLengthPrefixed(t *testing.T) {
+	bufCap := 10
+	inner := bytes.Repeat([]byte{0xff}, bufCap-2)
+	buf := make([]byte, 0, bufCap)
+	b := NewFixedBuilder(buf)
+	b.AddUint16LengthPrefixed(func(b *Builder) {
+		b.AddBytes(inner)
+	})
+	if got := b.BytesOrPanic(); len(got) != bufCap {
+		t.Errorf("Expected output lenght to be %d, got %d", bufCap, len(got))
+	}
+}
+
+func TestFixedBuilderPanicReallocate(t *testing.T) {
+	defer func() {
+		recover()
+	}()
+
+	b := NewFixedBuilder(make([]byte, 0, 10))
+	b1 := NewFixedBuilder(make([]byte, 0, 10))
+	b.AddUint16LengthPrefixed(func(b *Builder) {
+		*b = *b1
+	})
+
+	t.Error("Builder did not panic")
+}
+
 // ASN.1
 // ASN.1
 
 
 func TestASN1Int64(t *testing.T) {
 func TestASN1Int64(t *testing.T) {