From d0ce197c5835941df6d1d06017c1c34866a13650 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 14 Nov 2017 13:43:02 -0500 Subject: [PATCH] encoding/hex: make Decode, Decoder, DecodeString agree about partial results and errors CL 70210 added Decoder for #21590, and in doing so it changed the existing func Decode to return partial results for decoding errors. That seems like a good change to make to Decode, but it was untested (except as used by Decoder), inconsistent with DecodeString in all error cases, and inconsistent with Decoder in not returning partial results for odd-length input strings. This CL makes Decode, DecodeString, and Decoder all agree about the handling of partial results (they are returned) and error precedence (the error earliest in the input is reported), and it documents and tests this. Change-Id: Ifb7d1e100ecb66fe2ed5ba34a621084d480f16db Reviewed-on: https://go-review.googlesource.com/78120 Run-TryBot: Russ Cox Reviewed-by: Joe Tsai TryBot-Result: Gobot Gobot --- src/encoding/hex/hex.go | 42 +++++++++++++------- src/encoding/hex/hex_test.go | 75 ++++++++++++++++-------------------- 2 files changed, 61 insertions(+), 56 deletions(-) diff --git a/src/encoding/hex/hex.go b/src/encoding/hex/hex.go index f47b7fa34e..e4df6cbd4d 100644 --- a/src/encoding/hex/hex.go +++ b/src/encoding/hex/hex.go @@ -31,7 +31,9 @@ func Encode(dst, src []byte) int { return len(src) * 2 } -// ErrLength results from decoding an odd length slice. +// ErrLength reports an attempt to decode an odd-length input +// using Decode or DecodeString. +// The stream-based Decoder returns io.ErrUnexpectedEOF instead of ErrLength. var ErrLength = errors.New("encoding/hex: odd length hex string") // InvalidByteError values describe errors resulting from an invalid byte in a hex string. @@ -50,12 +52,11 @@ func DecodedLen(x int) int { return x / 2 } // // Decode expects that src contain only hexadecimal // characters and that src should have an even length. +// If the input is malformed, Decode returns the number +// of bytes decoded before the error. func Decode(dst, src []byte) (int, error) { - if len(src)%2 == 1 { - return 0, ErrLength - } - - for i := 0; i < len(src)/2; i++ { + var i int + for i = 0; i < len(src)/2; i++ { a, ok := fromHexChar(src[i*2]) if !ok { return i, InvalidByteError(src[i*2]) @@ -66,8 +67,15 @@ func Decode(dst, src []byte) (int, error) { } dst[i] = (a << 4) | b } - - return len(src) / 2, nil + if len(src)%2 == 1 { + // Check for invalid char before reporting bad length, + // since the invalid char (if present) is an earlier problem. + if _, ok := fromHexChar(src[i*2]); !ok { + return i, InvalidByteError(src[i*2]) + } + return i, ErrLength + } + return i, nil } // fromHexChar converts a hex character into its value and a success flag. @@ -92,15 +100,17 @@ func EncodeToString(src []byte) string { } // DecodeString returns the bytes represented by the hexadecimal string s. +// +// DecodeString expects that src contain only hexadecimal +// characters and that src should have an even length. +// If the input is malformed, DecodeString returns a string +// containing the bytes decoded before the error. func DecodeString(s string) ([]byte, error) { src := []byte(s) // We can use the source slice itself as the destination // because the decode loop increments by one and then the 'seen' byte is not used anymore. - len, err := Decode(src, src) - if err != nil { - return nil, err - } - return src[:len], nil + n, err := Decode(src, src) + return src[:n], err } // Dump returns a string that contains a hex dump of the given data. The format @@ -164,7 +174,11 @@ func (d *decoder) Read(p []byte) (n int, err error) { numRead, d.err = d.r.Read(d.arr[numCopy:]) d.in = d.arr[:numCopy+numRead] if d.err == io.EOF && len(d.in)%2 != 0 { - d.err = io.ErrUnexpectedEOF + if _, ok := fromHexChar(d.in[len(d.in)-1]); !ok { + d.err = InvalidByteError(d.in[len(d.in)-1]) + } else { + d.err = io.ErrUnexpectedEOF + } } } diff --git a/src/encoding/hex/hex_test.go b/src/encoding/hex/hex_test.go index d874b39e95..b6bab21c48 100644 --- a/src/encoding/hex/hex_test.go +++ b/src/encoding/hex/hex_test.go @@ -78,38 +78,37 @@ func TestDecodeString(t *testing.T) { } } -type errTest struct { +var errTests = []struct { in string + out string err error +}{ + {"", "", nil}, + {"0", "", ErrLength}, + {"zd4aa", "", InvalidByteError('z')}, + {"d4aaz", "\xd4\xaa", InvalidByteError('z')}, + {"30313", "01", ErrLength}, + {"0g", "", InvalidByteError('g')}, + {"00gg", "\x00", InvalidByteError('g')}, + {"0\x01", "", InvalidByteError('\x01')}, + {"ffeed", "\xff\xee", ErrLength}, } -var errTests = []errTest{ - {"0", ErrLength}, - {"zd4aa", ErrLength}, - {"0g", InvalidByteError('g')}, - {"00gg", InvalidByteError('g')}, - {"0\x01", InvalidByteError('\x01')}, -} - -func TestInvalidErr(t *testing.T) { - for i, test := range errTests { - dst := make([]byte, DecodedLen(len(test.in))) - _, err := Decode(dst, []byte(test.in)) - if err == nil { - t.Errorf("#%d: expected %v; got none", i, test.err) - } else if err != test.err { - t.Errorf("#%d: got: %v want: %v", i, err, test.err) +func TestDecodeErr(t *testing.T) { + for _, tt := range errTests { + out := make([]byte, len(tt.in)+10) + n, err := Decode(out, []byte(tt.in)) + if string(out[:n]) != tt.out || err != tt.err { + t.Errorf("Decode(%q) = %q, %v, want %q, %v", tt.in, string(out[:n]), err, tt.out, tt.err) } } } -func TestInvalidStringErr(t *testing.T) { - for i, test := range errTests { - _, err := DecodeString(test.in) - if err == nil { - t.Errorf("#%d: expected %v; got none", i, test.err) - } else if err != test.err { - t.Errorf("#%d: got: %v want: %v", i, err, test.err) +func TestDecodeStringErr(t *testing.T) { + for _, tt := range errTests { + out, err := DecodeString(tt.in) + if string(out) != tt.out || err != tt.err { + t.Errorf("DecodeString(%q) = %q, %v, want %q, %v", tt.in, out, err, tt.out, tt.err) } } } @@ -148,25 +147,17 @@ func TestEncoderDecoder(t *testing.T) { } } -func TestDecodeErr(t *testing.T) { - tests := []struct { - in string - wantOut string - wantErr error - }{ - {"", "", nil}, - {"0", "", io.ErrUnexpectedEOF}, - {"0g", "", InvalidByteError('g')}, - {"00gg", "\x00", InvalidByteError('g')}, - {"0\x01", "", InvalidByteError('\x01')}, - {"ffeed", "\xff\xee", io.ErrUnexpectedEOF}, - } - - for _, tt := range tests { +func TestDecoderErr(t *testing.T) { + for _, tt := range errTests { dec := NewDecoder(strings.NewReader(tt.in)) - got, err := ioutil.ReadAll(dec) - if string(got) != tt.wantOut || err != tt.wantErr { - t.Errorf("NewDecoder(%q) = (%q, %v), want (%q, %v)", tt.in, got, err, tt.wantOut, tt.wantErr) + out, err := ioutil.ReadAll(dec) + wantErr := tt.err + // Decoder is reading from stream, so it reports io.ErrUnexpectedEOF instead of ErrLength. + if wantErr == ErrLength { + wantErr = io.ErrUnexpectedEOF + } + if string(out) != tt.out || err != wantErr { + t.Errorf("NewDecoder(%q) = %q, %v, want %q, %v", tt.in, out, err, tt.out, wantErr) } } } -- 2.50.0