service: workaround for non-unicode strings in Variables (#4082)

When string Variables containing non-unicode strings are encoded into
json their value is changed, which leads to subtle bugs such as
described in issue #4072.
This CL addresses the problem by adding a new field to the api.Variable
struct that will be used to transport the variable's value for strings
that can not be unicode-encoded.
This is the best we can do without breaking backwards compatibility.
For our Go code the change is handled transparently by a
MarshalJSON/UnmarshalJSON on the api.Variable type.

Fixes #4072

Author: aarzilli

_fixtures/testvariables2.go

@@ -432,6 +432,7 @@ func main() {
 	var messageVar Message = Message{1, 2, 3, 4, 5}
 
 	var iface7 interface{} = OnlyUsedInInterface{"test"}
+	issue4072 := []byte{116, 121, 112, 101, 32, 84, 32, 115, 116, 114, 117, 99, 116, 32, 123, 12, 12, 9, 255, 102, 108, 100, 99, 111, 109, 255}
 
 	var amb1 = 1
 	runtime.Breakpoint()
@@ -443,5 +444,5 @@ func main() {
 	longslice := make([]int, 100, 100)
 
 	runtime.Breakpoint()
-	fmt.Println(i1, i2, i3, p1, pp1, amb1, s1, s3, a0, a1, p2, p3, s2, as1, str1, f1, fn1, fn2, nilslice, nilptr, ch1, chnil, m1, mnil, m2, m3, m4, m5, upnil, up1, i4, i5, i6, err1, err2, errnil, iface1, iface2, ifacenil, arr1, parr, cpx1, const1, iface3, iface4, recursive1, recursive1.x, iface5, iface2fn1, iface2fn2, bencharr, benchparr, mapinf, mainMenu, b, b2, sd, anonstruct1, anonstruct2, anoniface1, anonfunc, mapanonstruct1, ifacearr, efacearr, ni8, ni16, ni32, ni64, pinf, ninf, nan, zsvmap, zsslice, zsvar, tm, rettm, errtypednil, emptyslice, emptymap, byteslice, bytestypeslice, runeslice, bytearray, bytetypearray, runearray, longstr, nilstruct, as2, as2.NonPointerReceiverMethod, s4, iface2map, issue1578, ll, unread, w2, w3, w4, w5, longarr, longslice, val, m6, m7, cl, tim1, tim2, typedstringvar, namedA1, namedA2, astructName1(namedA2), badslice, tim3, int3chan, longbyteslice, enum1, enum2, enum3, enum4, enum5, enum6, zeropoint4, mlarge, messageVar, iface7)
+	fmt.Println(i1, i2, i3, p1, pp1, amb1, s1, s3, a0, a1, p2, p3, s2, as1, str1, f1, fn1, fn2, nilslice, nilptr, ch1, chnil, m1, mnil, m2, m3, m4, m5, upnil, up1, i4, i5, i6, err1, err2, errnil, iface1, iface2, ifacenil, arr1, parr, cpx1, const1, iface3, iface4, recursive1, recursive1.x, iface5, iface2fn1, iface2fn2, bencharr, benchparr, mapinf, mainMenu, b, b2, sd, anonstruct1, anonstruct2, anoniface1, anonfunc, mapanonstruct1, ifacearr, efacearr, ni8, ni16, ni32, ni64, pinf, ninf, nan, zsvmap, zsslice, zsvar, tm, rettm, errtypednil, emptyslice, emptymap, byteslice, bytestypeslice, runeslice, bytearray, bytetypearray, runearray, longstr, nilstruct, as2, as2.NonPointerReceiverMethod, s4, iface2map, issue1578, ll, unread, w2, w3, w4, w5, longarr, longslice, val, m6, m7, cl, tim1, tim2, typedstringvar, namedA1, namedA2, astructName1(namedA2), badslice, tim3, int3chan, longbyteslice, enum1, enum2, enum3, enum4, enum5, enum6, zeropoint4, mlarge, messageVar, iface7, issue4072)
 }

service/api/types.go

@@ -2,11 +2,13 @@ package api
 
 import (
 	"bytes"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"reflect"
 	"strconv"
 	"unicode"
+	"unicode/utf8"
 
 	"github.com/go-delve/delve/pkg/proc"
 )
@@ -328,6 +330,14 @@ type Variable struct {
 	// Function variables will store the name of the function in this field
 	Value string `json:"value"`
 
+	// StringValueBytes contains the Value for string variables that can not be
+	// represented in unicode.
+	// Specifically if v.Kind is reflect.String and v.Value is not a valid
+	// unicode string the JSON marshaller will convert it to a valid unicode
+	// string, altering its contents. This fields contains the unaltered string
+	// contents.
+	StringValueBytes []byte
+
 	// Number of elements in an array or a slice, number of keys for a map, number of struct members for a struct, length of strings, number of captured variables for functions
 	Len int64 `json:"len"`
 	// Cap value for slices
@@ -355,6 +365,36 @@ type Variable struct {
 	DeclLine int64
 }
 
+type variableWithoutMarshaler Variable
+
+func (v *Variable) MarshalJSON() (text []byte, err error) {
+	var v2 *variableWithoutMarshaler = (*variableWithoutMarshaler)(v)
+	if v2.Kind == reflect.String && !utf8.ValidString(v2.Value) {
+		value := v2.Value
+		// Should we blot out the value with something obviously wrong? Like:
+		//
+		// 	v2.Value = "<invalid-utf8-string>"
+		// 	defer func() {
+		// 		v2.Value = value
+		// 	}()
+		v2.StringValueBytes = []byte(value)
+	}
+	return json.Marshal(v2)
+}
+
+func (v *Variable) UnmarshalJSON(buf []byte) error {
+	var v2 *variableWithoutMarshaler = (*variableWithoutMarshaler)(v)
+	err := json.Unmarshal(buf, v2)
+	if err != nil {
+		return err
+	}
+	if v.Kind == reflect.String && len(v.StringValueBytes) > 0 {
+		v.Value = string(v.StringValueBytes)
+		v.StringValueBytes = nil
+	}
+	return nil
+}
+
 // LoadConfig describes how to load values from target's memory
 type LoadConfig struct {
 	// FollowPointers requests pointers to be automatically dereferenced.

service/test/integration2_test.go

@@ -3428,3 +3428,20 @@ func TestCancelDownload(t *testing.T) {
 		}
 	})
 }
+
+func TestEvalNonunicodeString(t *testing.T) {
+	// Non-unicode strings can not be sent through json encoding without being
+	// altered, check that our workaround works.
+	// See issue #4072.
+	withTestClient2("testvariables2", t, func(c service.Client) {
+		state := <-c.Continue()
+		assertNoError(state.Err, t, "Continue")
+		v, err := c.EvalVariable(api.EvalScope{GoroutineID: -1}, "string(issue4072)", normalLoadConfig)
+		assertNoError(err, t, "EvalVariable")
+		t.Logf("%s", v.MultilineString("", ""))
+		tgt := string([]byte{116, 121, 112, 101, 32, 84, 32, 115, 116, 114, 117, 99, 116, 32, 123, 12, 12, 9, 255, 102, 108, 100, 99, 111, 109, 255})
+		if v.Value != tgt {
+			t.Errorf("wrong value for string issue4072 expected %q, got %q", tgt, v.Value)
+		}
+	})
+}