icmp: fix miscalculation on multipart message bodies

Also adds test cases for the Len method of MessageBody interface.

Fixes golang/go#13141.

Change-Id: I8ab9e38727104ca094dfdb3020e8d42e611911e0
Reviewed-on: https://go-review.googlesource.com/16616
Reviewed-by: Ian Lance Taylor <iant@golang.org>

icmp/dstunreach.go

@@ -17,7 +17,7 @@ func (p *DstUnreach) Len(proto int) int {
 		return 0
 	}
 	l, _ := multipartMessageBodyDataLen(proto, p.Data, p.Extensions)
-	return l
+	return 4 + l
 }
 
 // Marshal implements the Marshal method of MessageBody interface.

icmp/multipart.go

@@ -35,7 +35,7 @@ func multipartMessageOrigDatagramLen(proto int, b []byte) int {
 			return 128
 		}
 		r := len(b)
-		return (r + align) &^ (align - 1)
+		return (r + align - 1) & ^(align - 1)
 	}
 	switch proto {
 	case iana.ProtocolICMP:

icmp/multipart_test.go

@@ -313,3 +313,130 @@ func dumpExtensions(i int, gotExts, wantExts []icmp.Extension) string {
 	}
 	return s[:len(s)-1]
 }
+
+var multipartMessageBodyLenTests = []struct {
+	proto int
+	in    icmp.MessageBody
+	out   int
+}{
+	{
+		iana.ProtocolICMP,
+		&icmp.DstUnreach{
+			Data: make([]byte, ipv4.HeaderLen),
+		},
+		4 + ipv4.HeaderLen, // unused and original datagram
+	},
+	{
+		iana.ProtocolICMP,
+		&icmp.TimeExceeded{
+			Data: make([]byte, ipv4.HeaderLen),
+		},
+		4 + ipv4.HeaderLen, // unused and original datagram
+	},
+	{
+		iana.ProtocolICMP,
+		&icmp.ParamProb{
+			Data: make([]byte, ipv4.HeaderLen),
+		},
+		4 + ipv4.HeaderLen, // [pointer, unused] and original datagram
+	},
+
+	{
+		iana.ProtocolICMP,
+		&icmp.ParamProb{
+			Data: make([]byte, ipv4.HeaderLen),
+			Extensions: []icmp.Extension{
+				&icmp.MPLSLabelStack{},
+			},
+		},
+		4 + 4 + 4 + 0 + 128, // [pointer, length, unused], extension header, object header, object payload, original datagram
+	},
+	{
+		iana.ProtocolICMP,
+		&icmp.ParamProb{
+			Data: make([]byte, 128),
+			Extensions: []icmp.Extension{
+				&icmp.MPLSLabelStack{},
+			},
+		},
+		4 + 4 + 4 + 0 + 128, // [pointer, length, unused], extension header, object header, object payload and original datagram
+	},
+	{
+		iana.ProtocolICMP,
+		&icmp.ParamProb{
+			Data: make([]byte, 129),
+			Extensions: []icmp.Extension{
+				&icmp.MPLSLabelStack{},
+			},
+		},
+		4 + 4 + 4 + 0 + 132, // [pointer, length, unused], extension header, object header, object payload and original datagram
+	},
+
+	{
+		iana.ProtocolIPv6ICMP,
+		&icmp.DstUnreach{
+			Data: make([]byte, ipv6.HeaderLen),
+		},
+		4 + ipv6.HeaderLen, // unused and original datagram
+	},
+	{
+		iana.ProtocolIPv6ICMP,
+		&icmp.PacketTooBig{
+			Data: make([]byte, ipv6.HeaderLen),
+		},
+		4 + ipv6.HeaderLen, // mtu and original datagram
+	},
+	{
+		iana.ProtocolIPv6ICMP,
+		&icmp.TimeExceeded{
+			Data: make([]byte, ipv6.HeaderLen),
+		},
+		4 + ipv6.HeaderLen, // unused and original datagram
+	},
+	{
+		iana.ProtocolIPv6ICMP,
+		&icmp.ParamProb{
+			Data: make([]byte, ipv6.HeaderLen),
+		},
+		4 + ipv6.HeaderLen, // pointer and original datagram
+	},
+
+	{
+		iana.ProtocolIPv6ICMP,
+		&icmp.DstUnreach{
+			Data: make([]byte, 127),
+			Extensions: []icmp.Extension{
+				&icmp.MPLSLabelStack{},
+			},
+		},
+		4 + 4 + 4 + 0 + 128, // [length, unused], extension header, object header, object payload and original datagram
+	},
+	{
+		iana.ProtocolIPv6ICMP,
+		&icmp.DstUnreach{
+			Data: make([]byte, 128),
+			Extensions: []icmp.Extension{
+				&icmp.MPLSLabelStack{},
+			},
+		},
+		4 + 4 + 4 + 0 + 128, // [length, unused], extension header, object header, object payload and original datagram
+	},
+	{
+		iana.ProtocolIPv6ICMP,
+		&icmp.DstUnreach{
+			Data: make([]byte, 129),
+			Extensions: []icmp.Extension{
+				&icmp.MPLSLabelStack{},
+			},
+		},
+		4 + 4 + 4 + 0 + 136, // [length, unused], extension header, object header, object payload and original datagram
+	},
+}
+
+func TestMultipartMessageBodyLen(t *testing.T) {
+	for i, tt := range multipartMessageBodyLenTests {
+		if out := tt.in.Len(tt.proto); out != tt.out {
+			t.Errorf("#%d: got %d; want %d", i, out, tt.out)
+		}
+	}
+}

icmp/paramprob.go

@@ -19,13 +19,13 @@ func (p *ParamProb) Len(proto int) int {
 		return 0
 	}
 	l, _ := multipartMessageBodyDataLen(proto, p.Data, p.Extensions)
-	return l
+	return 4 + l
 }
 
 // Marshal implements the Marshal method of MessageBody interface.
 func (p *ParamProb) Marshal(proto int) ([]byte, error) {
 	if proto == iana.ProtocolIPv6ICMP {
-		b := make([]byte, 4+p.Len(proto))
+		b := make([]byte, p.Len(proto))
 		b[0], b[1], b[2], b[3] = byte(p.Pointer>>24), byte(p.Pointer>>16), byte(p.Pointer>>8), byte(p.Pointer)
 		copy(b[4:], p.Data)
 		return b, nil

icmp/timeexceeded.go

@@ -16,7 +16,7 @@ func (p *TimeExceeded) Len(proto int) int {
 		return 0
 	}
 	l, _ := multipartMessageBodyDataLen(proto, p.Data, p.Extensions)
-	return l
+	return 4 + l
 }
 
 // Marshal implements the Marshal method of MessageBody interface.