Optimized marshaling of Go values into WAF values

The CPU profiler along with the new benchmark (agent side) allowed to spot every source of overhead.

The new implementation adds:

- requires at least go 1.12 which introduces reflect.Value.MapRange() which
  prevents using reflect.Value.MapKeys() that was creating a slice. Resulting
  in lots of overhead when large maps were traversed.

- strings are now limited to 4kB.

- remove every use of []byte(str) that in fact involve a copy of str into a
  new slice of bytes.

- the WAF value constructors are rewritten in Go to avoid short back-and-forth C
 calls that were adding a lot of CGO overhead (mainly pointer checks).

The benchmarks now gives a maximum time of 1ms.

azure-pipelines.yml

@@ -9,28 +9,27 @@ jobs:
       vmImage: ubuntu-latest
       go:
         container: golang
-        versions: [ 1.13, 1.12, 1.11 ]
+        versions: [ 1.13, 1.12 ]
         targets:
           GOARCH: [ amd64, 386 ]
 
   - template: tools/azure-pipelines/test-go-versions.yml
     parameters:
       name: Alpine_Linux
       vmImage: ubuntu-latest
-      continueOnError: true
       go:
         container: julio/azure-pipelines-golang
         versions: [ 1.13-alpine, 1.12-alpine ]
         targets:
-          GOARCH: [ amd64, 386 ]
+          GOARCH: [ amd64 ]
 
   - template: tools/azure-pipelines/test-go-versions.yml
     parameters:
       name: Windows
       vmImage: windows-latest
       go:
         container: golang
-        versions: [1.13, 1.12, 1.11]
+        versions: [ 1.13, 1.12 ]
         targets:
           GOARCH: [ amd64, 386 ]
 
@@ -41,7 +40,7 @@ jobs:
       - script: |
           go env
           clang -v
-          go test -v -x ./...
+          go test -v ./...
 
   - job: MacOS_386
     pool:
@@ -50,4 +49,4 @@ jobs:
       - script: |
           go env
           clang -v
-          env GOARCH=386 go test -v -x ./...
+          env GOARCH=386 go test -v ./...

go.mod

@@ -1,6 +1,6 @@
 module github.com/sqreen/go-libsqreen
 
-go 1.10
+go 1.12
 
 require (
 	github.com/pkg/errors v0.8.1

tools/azure-pipelines/test-go-versions.yml

@@ -4,7 +4,7 @@ parameters:
   continueOnError: false
   go:
     container: golang
-    versions: [ 1.13, 1.12, 1.11 ]
+    versions: [ 1.13, 1.12 ]
     targets:
       GOARCH: [ amd64 ]
 
@@ -26,4 +26,4 @@ jobs:
         GOARCH: $(GOARCH)
     steps:
       - script: go env
-      - script: go test -v -x ./...
+      - script: go test -v ./...

waf/internal/bindings/waf.go

@@ -10,12 +10,12 @@
 package bindings
 
 import (
-	"errors"
 	"fmt"
 	"reflect"
 	"time"
 	"unsafe"
 
+	"github.com/pkg/errors"
 	"github.com/sqreen/go-libsqreen/waf/types"
 )
 
@@ -63,14 +63,14 @@ func (r Rule) Close() error {
 	return nil
 }
 
-func (r Rule) Run(data types.RunInput, timeout time.Duration) (action types.Action, info []byte, err error) {
-	dataIn, err := WAFInput(data)
+func (r Rule) Run(data types.DataSet, timeout time.Duration) (action types.Action, info []byte, err error) {
+	wafValue, err := marshalWAFValue(data)
 	if err != nil {
 		return 0, nil, err
 	}
-	defer C.powerwaf_freeInput(dataIn, C.bool(false))
+	defer freeWAFValue(wafValue)
 
-	ret := C.powerwaf_runPowerWAF(r.id, dataIn, C.size_t(timeout/time.Microsecond))
+	ret := C.powerwaf_runPowerWAF(r.id, (*C.PWArgs)(wafValue), C.size_t(timeout/time.Microsecond))
 	defer C.powerwaf_freeReturn(ret)
 
 	switch a := ret.action; a {
@@ -113,65 +113,59 @@ func goRunError(cErr C.PW_RET_CODE, data *C.char) error {
 	return err
 }
 
-func WAFInput(data types.RunInput) (*C.PWArgs, error) {
-	return valueToWAFInput(reflect.ValueOf(data))
+type (
+	WAFValue    C.PWArgs
+	WAFInt      C.PWArgs
+	WAFUInt     C.PWArgs
+	WAFString   C.PWArgs
+	WAFMap      C.PWArgs
+	WAFMapEntry C.PWArgs
+	WAFArray    C.PWArgs
+)
+
+const maxWAFValueDepth = 10
+
+func marshalWAFValue(data types.DataSet) (*WAFValue, error) {
+	v := new(WAFValue)
+	if err := marshalWAFValueRec(reflect.ValueOf(data), v, maxWAFValueDepth); err != nil {
+		freeWAFValue(v)
+		return nil, err
+	}
+	return v, nil
 }
 
-func valueToWAFInput(v reflect.Value) (in *C.PWArgs, err error) {
-	switch v.Kind() {
+func marshalWAFValueRec(data reflect.Value, v *WAFValue, depth int) error {
+	if depth == 0 {
+		// Stop traversing and keep v to its current zero value.
+		return nil
+	}
+
+	switch data.Kind() {
 	default:
-		return nil, fmt.Errorf("unexpected WAF input type `%T`", v.Interface())
+		return fmt.Errorf("unexpected WAF input type `%T`", data.Interface())
 
 	case reflect.Ptr:
 		fallthrough
 	case reflect.Interface:
-		return valueToWAFInput(v.Elem())
+		// This interface or pointer traversal is not counted in the depth
+		return marshalWAFValueRec(data.Elem(), v, depth)
 
 	case reflect.String:
-		str := v.String()
-		cstr := C.CString(str)
-		defer C.free(unsafe.Pointer(cstr))
-		wstr := C.powerwaf_createStringWithLength(cstr, C.size_t(len(str)))
-		return &wstr, nil
+		return makeWAFString((*WAFString)(v), data.String())
 
 	case reflect.Map:
-		if v.Type().Key().Kind() != reflect.String {
-			return nil, fmt.Errorf("unexpected WAF map key type `%T` instead of `string`", v.Interface())
-		}
-		m := C.powerwaf_createMap()
-		in = &m
-		for _, k := range v.MapKeys() {
-			value, err := valueToWAFInput(v.MapIndex(k))
-			if err != nil {
-				C.powerwaf_freeInput(in, C.bool(false))
-				return nil, err
-			}
-			k := k.String()
-			key := C.CString(k)
-			defer C.free(unsafe.Pointer(key))
-			if !C.powerwaf_addToPWArgsMap(in, key, C.size_t(len(k)), *value) {
-				C.powerwaf_freeInput(value, C.bool(false))
-				C.powerwaf_freeInput(in, C.bool(false))
-				return nil, errors.New("could not insert a key element into a map")
-			}
+		if err := makeWAFMap((*WAFMap)(v), data.Len()); err != nil {
+			return err
 		}
-		return in, nil
+		return marshalWAFMap(data, (*WAFMap)(v), depth-1)
 
+	case reflect.Array:
+		fallthrough
 	case reflect.Slice:
-		a := C.powerwaf_createArray()
-		in = &a
-		for i := 0; i < v.Len(); i++ {
-			value, err := valueToWAFInput(v.Index(i))
-			if err != nil {
-				C.powerwaf_freeInput(in, C.bool(false))
-				return nil, err
-			}
-			if !C.powerwaf_addToPWArgsArray(in, *value) {
-				C.powerwaf_freeInput(in, C.bool(false))
-				return nil, fmt.Errorf("could not insert element `%d` of an array", i)
-			}
+		if err := makeWAFArray((*WAFArray)(v), data.Len()); err != nil {
+			return err
 		}
-		return in, nil
+		return marshalWAFArray(data, (*WAFArray)(v), depth-1)
 
 	case reflect.Int:
 		fallthrough
@@ -182,8 +176,7 @@ func valueToWAFInput(v reflect.Value) (in *C.PWArgs, err error) {
 	case reflect.Int32:
 		fallthrough
 	case reflect.Int64:
-		arg := C.powerwaf_createInt((C.int64_t)(v.Int()))
-		return &arg, nil
+		return makeWAFInt((*WAFInt)(v), data.Int())
 
 	case reflect.Uint:
 		fallthrough
@@ -194,16 +187,146 @@ func valueToWAFInput(v reflect.Value) (in *C.PWArgs, err error) {
 	case reflect.Uint32:
 		fallthrough
 	case reflect.Uint64:
-		arg := C.powerwaf_createUint((C.uint64_t)(v.Uint()))
-		return &arg, nil
+		return makeWAFUInt((*WAFUInt)(v), data.Uint())
 	}
 }
 
-//export goOnLogMessage
-func goOnLogMessage(level C.PW_LOG_LEVEL, _, _ *C.char, _ C.int, message *C.char, length C.size_t) {
-	fmt.Println(C.GoStringN(message, C.int(length)))
+func marshalWAFMap(data reflect.Value, v *WAFMap, depth int) error {
+	// Only allow string key types
+	if data.Type().Key().Kind() != reflect.String {
+		return errors.Errorf("unexpected WAF map key type `%T` instead of `string`", data.Interface())
+	}
+	// Marshal map entries
+	for i, iter := 0, data.MapRange(); iter.Next(); i++ {
+		entry := v.Index(i)
+		// Add the key first in order to get key insertion errors before traversing
+		// the value. It would be a waste if in the end the key cannot be added.
+		key := iter.Key().String()
+		if err := makeWAFMapKey(entry, key); err != nil {
+			return errors.Wrap(err, "could not add a new map key")
+		}
+		// Marshal the key's value
+		if err := marshalWAFValueRec(iter.Value(), (*WAFValue)(entry), depth); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func marshalWAFArray(data reflect.Value, v *WAFArray, depth int) error {
+	// Profiling shows `data.Len()` is called every loop if it is
+	// used in the loop condition.
+	l := data.Len()
+	for i := 0; i < l; i++ {
+		if err := marshalWAFValueRec(data.Index(i), v.Index(i), depth); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func makeWAFMap(v *WAFMap, len int) error {
+	return makeWAFLengthedValue((*WAFValue)(v), len, C.PWI_MAP)
 }
 
-func SetupLogging() {
-	C.powerwaf_setupLogging(C.powerwaf_logging_cb_t(C.onLogMessage), C.PWL_DEBUG)
+func (m *WAFMap) Index(i int) *WAFMapEntry {
+	entry := (*WAFArray)(m).Index(i)
+	return (*WAFMapEntry)(entry)
+}
+
+func (a *WAFArray) Index(i int) *WAFValue {
+	if C.uint64_t(i) >= a.nbEntries {
+		panic(errors.New("out of bounds access to WAFArray"))
+	}
+	// Go pointer arithmetic equivalent to the C expression
+	// `(PWArgs*)(a->value)[i]`
+	return (*WAFValue)(unsafe.Pointer(uintptr(a.value) + C.sizeof_PWArgs*uintptr(i)))
+}
+
+func makeWAFMapKey(v *WAFMapEntry, key string) error {
+	cstr, length := cstring(key)
+	if cstr == nil {
+		return types.ErrOutOfMemory
+	}
+	v.parameterName = cstr
+	v.parameterNameLength = C.uint64_t(length)
+	return nil
+}
+
+func makeWAFArray(v *WAFArray, len int) error {
+	return makeWAFLengthedValue((*WAFValue)(v), len, C.PWI_ARRAY)
+}
+
+const maxWAFStringSize = 4 * 1024
+
+func makeWAFString(v *WAFString, str string) error {
+	cstr, length := cstring(str)
+	if cstr == nil {
+		return types.ErrOutOfMemory
+	}
+	v.value = unsafe.Pointer(cstr)
+	v.nbEntries = C.uint64_t(length)
+	v._type = C.PWI_STRING
+	return nil
+}
+
+// cstring returns the C string of the given Go string `str` with up to
+// maxWAFStringSize bytes, along with the string size that was copied.
+func cstring(str string) (*C.char, int) {
+	// Limit the maximum string size to copy
+	l := len(str)
+	if l > maxWAFStringSize {
+		l = maxWAFStringSize
+	}
+	// Copy the string up to l.
+	// The copy is required as the pointer will be stored into the C structures,
+	// so using a Go pointer is impossible (and detected by the cgo pointer checks
+	// anyway).
+	return C.CString(str[:l]), l
+}
+
+func makeWAFInt(v *WAFInt, n int64) error {
+	return makeWAFBasicValue((*WAFValue)(v), uintptr(n), C.PWI_SIGNED_NUMBER)
+}
+
+func makeWAFUInt(v *WAFUInt, n uint64) error {
+	return makeWAFBasicValue((*WAFValue)(v), uintptr(n), C.PWI_UNSIGNED_NUMBER)
+}
+
+func makeWAFBasicValue(v *WAFValue, data uintptr, wafType C.PW_INPUT_TYPE) error {
+	v.value = unsafe.Pointer(data)
+	v._type = wafType
+	return nil
+}
+
+func makeWAFLengthedValue(v *WAFValue, len int, wafType C.PW_INPUT_TYPE) error {
+	// Allocate the zero'd array.
+	a := C.calloc(C.size_t(len), C.sizeof_PWArgs)
+	if a == nil {
+		return types.ErrOutOfMemory
+	}
+
+	v.value = a
+	v.nbEntries = C.uint64_t(len)
+	v._type = wafType
+	return nil
+}
+
+func freeWAFValue(v *WAFValue) {
+	switch v._type {
+	case C.PWI_MAP:
+		fallthrough
+	case C.PWI_ARRAY:
+		for child := 0; C.uint64_t(child) < v.nbEntries; child++ {
+			entry := (*WAFArray)(v).Index(child)
+			if entry.parameterName != nil {
+				C.free(unsafe.Pointer(entry.parameterName))
+			}
+			freeWAFValue(entry)
+		}
+	}
+
+	if v.value != nil {
+		C.free(v.value)
+	}
 }