encoding/json: fix handling of null with ,string fields

Fixes issue 8587.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews, iant, r
https://codereview.appspot.com/152270044

2 files changed, 50 insertions, 11 deletions

diff --git a/src/encoding/json/decode.go b/src/encoding/json/decode.go
index 67ec37388..705bc2e17 100644
--- a/src/encoding/json/decode.go
+++ b/src/encoding/json/decode.go
@@ -173,7 +173,6 @@ type decodeState struct {
 	scan       scanner
 	nextscan   scanner // for calls to nextValue
 	savedError error
-	tempstr    string // scratch space to avoid some allocations
 	useNumber  bool
 }
 
@@ -293,6 +292,32 @@ func (d *decodeState) value(v reflect.Value) {
 	}
 }
 
+type unquotedValue struct{}
+
+// valueQuoted is like value but decodes a
+// quoted string literal or literal null into an interface value.
+// If it finds anything other than a quoted string literal or null,
+// valueQuoted returns unquotedValue{}.
+func (d *decodeState) valueQuoted() interface{} {
+	switch op := d.scanWhile(scanSkipSpace); op {
+	default:
+		d.error(errPhase)
+
+	case scanBeginArray:
+		d.array(reflect.Value{})
+
+	case scanBeginObject:
+		d.object(reflect.Value{})
+
+	case scanBeginLiteral:
+		switch v := d.literalInterface().(type) {
+		case nil, string:
+			return v
+		}
+	}
+	return unquotedValue{}
+}
+
 // indirect walks down v allocating pointers as needed,
 // until it gets to a non-pointer.
 // if it encounters an Unmarshaler, indirect stops and returns that.
@@ -444,6 +469,8 @@ func (d *decodeState) array(v reflect.Value) {
 	}
 }
 
+var nullLiteral = []byte("null")
+
 // object consumes an object from d.data[d.off-1:], decoding into the value v.
 // the first byte ('{') of the object has been read already.
 func (d *decodeState) object(v reflect.Value) {
@@ -566,9 +593,14 @@ func (d *decodeState) object(v reflect.Value) {
 
 		// Read value.
 		if destring {
-			d.value(reflect.ValueOf(&d.tempstr))
-			d.literalStore([]byte(d.tempstr), subv, true)
-			d.tempstr = "" // Zero scratch space for successive values.
+			switch qv := d.valueQuoted().(type) {
+			case nil:
+				d.literalStore(nullLiteral, subv, false)
+			case string:
+				d.literalStore([]byte(qv), subv, true)
+			default:
+				d.saveError(fmt.Errorf("json: invalid use of ,string struct tag, trying to unmarshal unquoted value into %v", item, v.Type()))
+			}
 		} else {
 			d.value(subv)
 		}
diff --git a/src/encoding/json/decode_test.go b/src/encoding/json/decode_test.go
index d95657d72..7235969b9 100644
--- a/src/encoding/json/decode_test.go
+++ b/src/encoding/json/decode_test.go
@@ -1070,18 +1070,25 @@ func TestEmptyString(t *testing.T) {
 	}
 }
 
-// Test that the returned error is non-nil when trying to unmarshal null string into int, for successive ,string option
-// Issue 7046
+// Test that a null for ,string is not replaced with the previous quoted string (issue 7046).
+// It should also not be an error (issue 2540, issue 8587).
 func TestNullString(t *testing.T) {
 	type T struct {
-		A int `json:",string"`
-		B int `json:",string"`
+		A int  `json:",string"`
+		B int  `json:",string"`
+		C *int `json:",string"`
 	}
-	data := []byte(`{"A": "1", "B": null}`)
+	data := []byte(`{"A": "1", "B": null, "C": null}`)
 	var s T
+	s.B = 1
+	s.C = new(int)
+	*s.C = 2
 	err := Unmarshal(data, &s)
-	if err == nil {
-		t.Fatalf("expected error; got %v", s)
+	if err != nil {
+		t.Fatalf("Unmarshal: %v")
+	}
+	if s.B != 1 || s.C != nil {
+		t.Fatalf("after Unmarshal, s.B=%d, s.C=%p, want 1, nil", s.B, s.C)
 	}
 }