all: remove non-fatal UTF-8 validation errors (and non-fatal in general)
Immediately abort (un)marshal operations when encountering invalid UTF-8
data in proto3 strings. No other proto implementation supports non-UTF-8
data in proto3 strings (and many reject it in proto2 strings as well).
Producing invalid output is an interoperability threat (other
implementations won't be able to read it).
The case where existing string data is found to contain non-UTF8 data is
better handled by changing the field to the `bytes` type, which (aside
from UTF-8 validation) is wire-compatible with `string`.
Remove the errors.NonFatal type, since there are no remaining cases
where it is needed. "Non-fatal" errors which produce results and a
non-nil error are problematic because they compose poorly; the better
approach is to take an option like AllowPartial indicating which
conditions to check for.
Change-Id: I9d189ec6ffda7b5d96d094aa1b290af2e3f23736
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/183098
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
diff --git a/internal/encoding/json/decode.go b/internal/encoding/json/decode.go
index 27dff34..ae2ee9a 100644
--- a/internal/encoding/json/decode.go
+++ b/internal/encoding/json/decode.go
@@ -69,9 +69,8 @@
return d.value, d.err
}
- var nerr errors.NonFatal
value, err := d.parseNext()
- if !nerr.Merge(err) {
+ if err != nil {
return Value{}, err
}
n := value.size
@@ -145,7 +144,7 @@
if d.value.typ == comma {
return d.Read()
}
- return value, nerr.E
+ return value, nil
}
// Any sequence that looks like a non-delimiter (for error reporting).
@@ -193,12 +192,11 @@
return d.newValue(Number, in, n), nil
case '"':
- var nerr errors.NonFatal
s, n, err := d.parseString(in)
- if !nerr.Merge(err) {
+ if err != nil {
return Value{}, err
}
- return d.newStringValue(in, n, s), nerr.E
+ return d.newStringValue(in, n, s), nil
case '{':
return d.newValue(StartObject, in, 1), nil
diff --git a/internal/encoding/json/decode_test.go b/internal/encoding/json/decode_test.go
index daf3f7c..c29d508 100644
--- a/internal/encoding/json/decode_test.go
+++ b/internal/encoding/json/decode_test.go
@@ -121,10 +121,7 @@
{
// Invalid UTF-8 error is returned in ReadString instead of Read.
input: "\"\xff\"",
- want: []R{
- {T: json.String, E: `invalid UTF-8 detected`, V: string("\xff")},
- {T: json.EOF},
- },
+ want: []R{{E: `syntax error (line 1:1): invalid UTF-8 in string`}},
},
{
input: `"` + string(utf8.RuneError) + `"`,
diff --git a/internal/encoding/json/encode_test.go b/internal/encoding/json/encode_test.go
index 3f85f65..b98f98b 100644
--- a/internal/encoding/json/encode_test.go
+++ b/internal/encoding/json/encode_test.go
@@ -349,19 +349,6 @@
]
]
}`,
- },
- {
- desc: "string contains rune error",
- write: func(e *json.Encoder) {
- // WriteString returns non-fatal error for invalid UTF sequence, but
- // should still output the written value. See TestWriteStringError
- // below that checks for this.
- e.StartObject()
- e.WriteName("invalid rune")
- e.WriteString("abc\xff")
- e.EndObject()
- },
- wantOut: "{\"invalid rune\":\"abc\xff\"}",
}}
for _, tc := range tests {
diff --git a/internal/encoding/json/string.go b/internal/encoding/json/string.go
index 1d89aca..b63dd5c 100644
--- a/internal/encoding/json/string.go
+++ b/internal/encoding/json/string.go
@@ -16,15 +16,13 @@
)
func appendString(out []byte, in string) ([]byte, error) {
- var nerr errors.NonFatal
out = append(out, '"')
i := indexNeedEscapeInString(in)
in, out = in[i:], append(out, in[:i]...)
for len(in) > 0 {
switch r, n := utf8.DecodeRuneInString(in); {
case r == utf8.RuneError && n == 1:
- nerr.AppendInvalidUTF8("")
- in, out = in[1:], append(out, in[0]) // preserve invalid byte
+ return out, errors.InvalidUTF8("")
case r < ' ' || r == '"' || r == '\\':
out = append(out, '\\')
switch r {
@@ -52,11 +50,10 @@
}
}
out = append(out, '"')
- return out, nerr.E
+ return out, nil
}
func (d *Decoder) parseString(in []byte) (string, int, error) {
- var nerr errors.NonFatal
in0 := in
if len(in) == 0 {
return "", 0, io.ErrUnexpectedEOF
@@ -70,14 +67,13 @@
for len(in) > 0 {
switch r, n := utf8.DecodeRune(in); {
case r == utf8.RuneError && n == 1:
- nerr.AppendInvalidUTF8("")
- in, out = in[1:], append(out, in[0]) // preserve invalid byte
+ return "", 0, d.newSyntaxError("invalid UTF-8 in string")
case r < ' ':
return "", 0, d.newSyntaxError("invalid character %q in string", r)
case r == '"':
in = in[1:]
n := len(in0) - len(in)
- return string(out), n, nerr.E
+ return string(out), n, nil
case r == '\\':
if len(in) < 2 {
return "", 0, io.ErrUnexpectedEOF
diff --git a/internal/encoding/text/decode.go b/internal/encoding/text/decode.go
index 0babddf..2b32ed9 100644
--- a/internal/encoding/text/decode.go
+++ b/internal/encoding/text/decode.go
@@ -26,7 +26,7 @@
p := decoder{in: b}
p.consume(0) // trim leading spaces or comments
v, err := p.unmarshalMessage(false)
- if !p.nerr.Merge(err) {
+ if err != nil {
if e, ok := err.(syntaxError); ok {
b = b[:len(b)-len(p.in)] // consumed input
line := bytes.Count(b, []byte("\n")) + 1
@@ -41,12 +41,11 @@
if len(p.in) > 0 {
return Value{}, errors.New("%d bytes of unconsumed input", len(p.in))
}
- return v, p.nerr.E
+ return v, nil
}
type decoder struct {
- nerr errors.NonFatal
- in []byte
+ in []byte
}
func (p *decoder) unmarshalList() (Value, error) {
@@ -58,7 +57,7 @@
if len(p.in) > 0 && p.in[0] != ']' {
for len(p.in) > 0 {
v, err := p.unmarshalValue()
- if !p.nerr.Merge(err) {
+ if err != nil {
return Value{}, err
}
elems = append(elems, v)
@@ -91,14 +90,14 @@
break
}
k, err := p.unmarshalKey()
- if !p.nerr.Merge(err) {
+ if err != nil {
return Value{}, err
}
if !p.tryConsumeChar(':') && len(p.in) > 0 && p.in[0] != '{' && p.in[0] != '<' {
return Value{}, newSyntaxError("expected ':' after message key")
}
v, err := p.unmarshalValue()
- if !p.nerr.Merge(err) {
+ if err != nil {
return Value{}, err
}
if p.tryConsumeChar(';') || p.tryConsumeChar(',') {
@@ -132,7 +131,7 @@
// This is specific to Go and contrary to the C++ implementation,
// which does not support strings for the Any type URL.
v, err = p.unmarshalString()
- if !p.nerr.Merge(err) {
+ if err != nil {
return Value{}, err
}
} else if n := matchWithDelim(urlRegexp, p.in); n > 0 {
diff --git a/internal/encoding/text/encode.go b/internal/encoding/text/encode.go
index c6878ee..982037b 100644
--- a/internal/encoding/text/encode.go
+++ b/internal/encoding/text/encode.go
@@ -45,18 +45,17 @@
p.outputASCII = outputASCII
err := p.marshalMessage(v, false)
- if !p.nerr.Merge(err) {
+ if err != nil {
return nil, err
}
if len(indent) > 0 {
- return append(bytes.TrimRight(p.out, "\n"), '\n'), p.nerr.E
+ return append(bytes.TrimRight(p.out, "\n"), '\n'), nil
}
- return p.out, p.nerr.E
+ return p.out, nil
}
type encoder struct {
- nerr errors.NonFatal
- out []byte
+ out []byte
indent string
indents []byte
@@ -77,7 +76,7 @@
}
for i, elem := range elems {
p.out = append(p.out, p.indents...)
- if err := p.marshalValue(elem); !p.nerr.Merge(err) {
+ if err := p.marshalValue(elem); err != nil {
return err
}
if i < len(elems)-1 {
@@ -107,7 +106,7 @@
}
for i, item := range items {
p.out = append(p.out, p.indents...)
- if err := p.marshalKey(item[0]); !p.nerr.Merge(err) {
+ if err := p.marshalKey(item[0]); err != nil {
return err
}
p.out = append(p.out, ':')
@@ -120,7 +119,7 @@
p.out = append(p.out, ' ')
}
- if err := p.marshalValue(item[1]); !p.nerr.Merge(err) {
+ if err := p.marshalValue(item[1]); err != nil {
return err
}
if i < len(items)-1 && len(p.indent) == 0 {
diff --git a/internal/encoding/text/string.go b/internal/encoding/text/string.go
index 56f13cd..6792e7a 100644
--- a/internal/encoding/text/string.go
+++ b/internal/encoding/text/string.go
@@ -86,7 +86,6 @@
return v, err
}
func consumeString(in []byte) (Value, int, error) {
- var nerr errors.NonFatal
in0 := in
if len(in) == 0 {
return Value{}, 0, io.ErrUnexpectedEOF
@@ -101,15 +100,14 @@
for len(in) > 0 {
switch r, n := utf8.DecodeRune(in); {
case r == utf8.RuneError && n == 1:
- nerr.AppendInvalidUTF8("")
- in, out = in[1:], append(out, in[0]) // preserve invalid byte
+ return Value{}, 0, newSyntaxError("invalid UTF-8 detected")
case r == 0 || r == '\n':
return Value{}, 0, newSyntaxError("invalid character %q in string", r)
case r == rune(quote):
in = in[1:]
n := len(in0) - len(in)
v := rawValueOf(string(out), in0[:n:n])
- return v, n, nerr.E
+ return v, n, nil
case r == '\\':
if len(in) < 2 {
return Value{}, 0, io.ErrUnexpectedEOF
@@ -208,7 +206,7 @@
var ss []string
for len(p.in) > 0 && (p.in[0] == '"' || p.in[0] == '\'') {
v, err := p.unmarshalString()
- if !p.nerr.Merge(err) {
+ if err != nil {
return Value{}, err
}
ss = append(ss, v.String())
diff --git a/internal/encoding/text/text_test.go b/internal/encoding/text/text_test.go
index 97caa74..ee43190 100644
--- a/internal/encoding/text/text_test.go
+++ b/internal/encoding/text/text_test.go
@@ -354,32 +354,20 @@
wantOut: `str:"\x01\x02\x03\x04\x05\x06\x07\x08\t\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !\"#$%&()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_` + "`abcdefghijklmnopqrstuvwxyz{|}~\x7f\"",
wantOutASCII: `str:"\x01\x02\x03\x04\x05\x06\x07\x08\t\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !\"#$%&()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_` + "`abcdefghijklmnopqrstuvwxyz{|}~\x7f\"",
}, {
- in: "str: '\xde\xad\xbe\xef'",
- wantVal: V(Msg{{ID("str"), V("\xde\xad\xbe\xef")}}),
- wantOut: "str:\"\u07ad\\xbe\\xef\"",
- wantOutASCII: `str:"\u07ad\xbe\xef"`,
- wantErr: "invalid UTF-8 detected",
+ in: "str: '\xde\xad\xbe\xef'",
+ wantErr: "invalid UTF-8 detected",
}, {
// Valid UTF-8 wire encoding, but sub-optimal encoding.
- in: "str: '\xc0\x80'",
- wantVal: V(Msg{{ID("str"), V("\xc0\x80")}}),
- wantOut: `str:"\xc0\x80"`,
- wantOutASCII: `str:"\xc0\x80"`,
- wantErr: "invalid UTF-8 detected",
+ in: "str: '\xc0\x80'",
+ wantErr: "invalid UTF-8 detected",
}, {
// Valid UTF-8 wire encoding, but invalid rune (surrogate pair).
- in: "str: '\xed\xa0\x80'",
- wantVal: V(Msg{{ID("str"), V("\xed\xa0\x80")}}),
- wantOut: `str:"\xed\xa0\x80"`,
- wantOutASCII: `str:"\xed\xa0\x80"`,
- wantErr: "invalid UTF-8 detected",
+ in: "str: '\xed\xa0\x80'",
+ wantErr: "invalid UTF-8 detected",
}, {
// Valid UTF-8 wire encoding, but invalid rune (above max rune).
- in: "str: '\xf7\xbf\xbf\xbf'",
- wantVal: V(Msg{{ID("str"), V("\xf7\xbf\xbf\xbf")}}),
- wantOut: `str:"\xf7\xbf\xbf\xbf"`,
- wantOutASCII: `str:"\xf7\xbf\xbf\xbf"`,
- wantErr: "invalid UTF-8 detected",
+ in: "str: '\xf7\xbf\xbf\xbf'",
+ wantErr: "invalid UTF-8 detected",
}, {
// Valid UTF-8 wire encoding of the RuneError rune.
in: "str: '\xef\xbf\xbd'",
diff --git a/internal/encoding/text/value.go b/internal/encoding/text/value.go
index 8387707..b092598 100644
--- a/internal/encoding/text/value.go
+++ b/internal/encoding/text/value.go
@@ -327,7 +327,7 @@
return v.raw
}
p := encoder{}
- if err := p.marshalValue(v); !p.nerr.Merge(err) {
+ if err := p.marshalValue(v); err != nil {
return []byte("<invalid>")
}
return p.out