Merge pull request #139 from digitalocean/anitgandhi/no-unsafe

ovsh: replace usage of unsafe package with binary marshaling and unmarshaling

Author: anitgandhi

ovsnl/client.go

@@ -18,20 +18,11 @@ import (
 	"fmt"
 	"os"
 	"strings"
-	"unsafe"
 
 	"github.com/digitalocean/go-openvswitch/ovsnl/internal/ovsh"
 	"github.com/mdlayher/genetlink"
 )
 
-// Sizes of various structures, used in unsafe casts.
-const (
-	sizeofHeader = int(unsafe.Sizeof(ovsh.Header{}))
-
-	sizeofDPStats         = int(unsafe.Sizeof(ovsh.DPStats{}))
-	sizeofDPMegaflowStats = int(unsafe.Sizeof(ovsh.DPMegaflowStats{}))
-)
-
 // A Client is a Linux Open vSwitch generic netlink client.
 type Client struct {
 	// Datapath provides access to DatapathService methods.
@@ -116,20 +107,3 @@ func (c *Client) initFamily(f genetlink.Family) error {
 		return fmt.Errorf("unknown OVS generic netlink family: %q", f.Name)
 	}
 }
-
-// headerBytes converts an ovsh.Header into a byte slice.
-func headerBytes(h ovsh.Header) []byte {
-	b := *(*[sizeofHeader]byte)(unsafe.Pointer(&h)) // #nosec G103
-	return b[:]
-}
-
-// parseHeader converts a byte slice into ovsh.Header.
-func parseHeader(b []byte) (ovsh.Header, error) {
-	// Verify that the byte slice is long enough before doing unsafe casts.
-	if l := len(b); l < sizeofHeader {
-		return ovsh.Header{}, fmt.Errorf("not enough data for OVS message header: %d bytes", l)
-	}
-
-	h := *(*ovsh.Header)(unsafe.Pointer(&b[:sizeofHeader][0])) // #nosec G103
-	return h, nil
-}

ovsnl/datapath.go

@@ -15,8 +15,7 @@
 package ovsnl
 
 import (
-	"fmt"
-	"unsafe"
+	"encoding/binary"
 
 	"github.com/digitalocean/go-openvswitch/ovsnl/internal/ovsh"
 	"github.com/mdlayher/genetlink"
@@ -98,15 +97,20 @@ type DatapathMegaflowStats struct {
 
 // List lists all Datapaths in the kernel.
 func (s *DatapathService) List() ([]Datapath, error) {
+	data, err := ovsh.MarshalBinary(&ovsh.Header{
+		// Query all datapaths.
+		Ifindex: 0,
+	})
+	if err != nil {
+		return nil, err
+	}
+
 	req := genetlink.Message{
 		Header: genetlink.Header{
 			Command: ovsh.DpCmdGet,
 			Version: uint8(s.f.Version),
 		},
-		// Query all datapaths.
-		Data: headerBytes(ovsh.Header{
-			Ifindex: 0,
-		}),
+		Data: data,
 	}
 
 	flags := netlink.Request | netlink.Dump
@@ -134,8 +138,9 @@ func parseDatapaths(msgs []genetlink.Message) ([]Datapath, error) {
 			Index: int(h.Ifindex),
 		}
 
-		// Skip the header to parse attributes.
-		attrs, err := netlink.UnmarshalAttributes(m.Data[sizeofHeader:])
+		// Skip past the header to parse attributes.
+		hdrSize := binary.Size(h)
+		attrs, err := netlink.UnmarshalAttributes(m.Data[hdrSize:])
 		if err != nil {
 			return nil, err
 		}
@@ -167,13 +172,12 @@ func parseDatapaths(msgs []genetlink.Message) ([]Datapath, error) {
 
 // parseDPStats converts a byte slice into DatapathStats.
 func parseDPStats(b []byte) (DatapathStats, error) {
-	// Verify that the byte slice is the correct length before doing
-	// unsafe casts.
-	if want, got := sizeofDPStats, len(b); want != got {
-		return DatapathStats{}, fmt.Errorf("unexpected datapath stats structure size, want %d, got %d", want, got)
+	s := new(ovsh.DPStats)
+	err := ovsh.UnmarshalBinary(b, s)
+	if err != nil {
+		return DatapathStats{}, err
 	}
 
-	s := *(*ovsh.DPStats)(unsafe.Pointer(&b[0])) // #nosec G103
 	return DatapathStats{
 		Hit:    s.Hit,
 		Missed: s.Missed,
@@ -184,16 +188,24 @@ func parseDPStats(b []byte) (DatapathStats, error) {
 
 // parseDPMegaflowStats converts a byte slice into DatapathMegaflowStats.
 func parseDPMegaflowStats(b []byte) (DatapathMegaflowStats, error) {
-	// Verify that the byte slice is the correct length before doing
-	// unsafe casts.
-	if want, got := sizeofDPMegaflowStats, len(b); want != got {
-		return DatapathMegaflowStats{}, fmt.Errorf("unexpected datapath megaflow stats structure size, want %d, got %d", want, got)
+	s := new(ovsh.DPMegaflowStats)
+	err := ovsh.UnmarshalBinary(b, s)
+	if err != nil {
+		return DatapathMegaflowStats{}, err
 	}
 
-	s := *(*ovsh.DPMegaflowStats)(unsafe.Pointer(&b[0])) // #nosec G103
-
 	return DatapathMegaflowStats{
 		MaskHits: s.Mask_hit,
 		Masks:    s.Masks,
 	}, nil
 }
+
+// parseHeader converts a byte slice into ovsh.Header.
+func parseHeader(b []byte) (ovsh.Header, error) {
+	h := new(ovsh.Header)
+	err := ovsh.UnmarshalBinary(b, h)
+	if err != nil {
+		return ovsh.Header{}, err
+	}
+	return *h, nil
+}

ovsnl/datapath_linux_test.go

@@ -13,12 +13,12 @@
 // limitations under the License.
 
 //go:build linux
+// +build linux
 
 package ovsnl
 
 import (
 	"testing"
-	"unsafe"
 
 	"github.com/digitalocean/go-openvswitch/ovsnl/internal/ovsh"
 	"github.com/google/go-cmp/cmp"
@@ -136,7 +136,8 @@ func TestClientDatapathListOK(t *testing.T) {
 			t.Fatalf("unexpected generic netlink command (-want +got):\n%s", diff)
 		}
 
-		h, err := parseHeader(greq.Data)
+		h := new(ovsh.Header)
+		err := ovsh.UnmarshalBinary(greq.Data, h)
 		if err != nil {
 			t.Fatalf("failed to parse OvS generic netlink header: %v", err)
 		}
@@ -147,7 +148,7 @@ func TestClientDatapathListOK(t *testing.T) {
 
 		return []genetlink.Message{
 			{
-				Data: mustMarshalDatapath(system),
+				Data: mustMarshalDatapath(t, system),
 			},
 		}, nil
 	}))
@@ -172,29 +173,34 @@ func TestClientDatapathListOK(t *testing.T) {
 	}
 }
 
-func mustMarshalDatapath(dp Datapath) []byte {
-	h := ovsh.Header{
+func mustMarshalDatapath(t *testing.T, dp Datapath) []byte {
+	hb, err := ovsh.MarshalBinary(&ovsh.Header{
 		Ifindex: int32(dp.Index),
+	})
+	if err != nil {
+		t.Fatal(t)
 	}
 
-	hb := headerBytes(h)
-
-	s := ovsh.DPStats{
+	sb, err := ovsh.MarshalBinary(&ovsh.DPStats{
 		Hit:    dp.Stats.Hit,
 		Missed: dp.Stats.Missed,
 		Lost:   dp.Stats.Lost,
 		Flows:  dp.Stats.Flows,
+	})
+	if err != nil {
+		t.Fatal(t)
 	}
 
-	sb := *(*[sizeofDPStats]byte)(unsafe.Pointer(&s)) // #nosec G103
-
 	ms := ovsh.DPMegaflowStats{
 		Mask_hit: dp.MegaflowStats.MaskHits,
 		Masks:    dp.MegaflowStats.Masks,
 		// Pad already set to zero.
 	}
 
-	msb := *(*[sizeofDPMegaflowStats]byte)(unsafe.Pointer(&ms)) // #nosec G103
+	msb, err := ovsh.MarshalBinary(&ms)
+	if err != nil {
+		t.Fatal(t)
+	}
 
 	ab := mustMarshalAttributes([]netlink.Attribute{
 		{
@@ -207,7 +213,7 @@ func mustMarshalDatapath(dp Datapath) []byte {
 		},
 		{
 			Type: ovsh.DpAttrStats,
-			Data: sb[:],
+			Data: sb,
 		},
 		{
 			Type: ovsh.DpAttrMegaflowStats,

ovsnl/internal/ovsh/binary.go

@@ -0,0 +1,45 @@
+// Copyright 2017 DigitalOcean.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package ovsh
+
+import (
+	"bytes"
+	"encoding/binary"
+	"fmt"
+)
+
+// ovsTypes defines a type set of the types defined in struct.go
+type ovsTypes interface {
+	Header | DPStats | DPMegaflowStats | VportStats | FlowStats
+}
+
+// MarshalBinary is a generic binary marshaling function for the type defined in struct.go
+func MarshalBinary[T ovsTypes](data *T) ([]byte, error) {
+	var buf bytes.Buffer
+	err := binary.Write(&buf, binary.NativeEndian, data)
+	return buf.Bytes(), err
+}
+
+// UnmarshalBinary is a generic binary unmarshaling function for the type defined in struct.go
+func UnmarshalBinary[T ovsTypes](data []byte, dst *T) error {
+	// Verify that the byte slice has enough data before unmarshaling.
+	if want, got := binary.Size(*dst), len(data); got < want {
+		return fmt.Errorf("unexpected size of struct %T, want at least %d, got %d", *dst, want, got)
+	}
+
+	*dst = *new(T) // reset the contents, just to be safe
+	buf := bytes.NewBuffer(data)
+	return binary.Read(buf, binary.NativeEndian, dst)
+}