Skip to content

Commit

Permalink
Retry in decoder if Read() returns 0 bytes read with nil error (#387)
Browse files Browse the repository at this point in the history 
Previously, decoder did not properly handle the scenario of
Read() returning 0 bytes read with nil error.

Fix this by retrying the read on (0, nil).

While at it, also refactor the error handling and add
more tests to reach 100% code coverage for stream.go.
  • Loading branch information
@fxamacker
fxamacker authored Feb 20, 2023
1 parent 4b3a963 commit f9e6291
Show file tree
Hide file tree
Showing 3 changed files with 621 additions and 288 deletions.
53 changes: 27 additions & 26 deletions decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,17 @@ import (
// To unmarshal CBOR into an empty interface value, Unmarshal uses the
// following rules:
//
// CBOR booleans decode to bool.
// CBOR positive integers decode to uint64.
// CBOR negative integers decode to int64 (big.Int if value overflows).
// CBOR floating points decode to float64.
// CBOR byte strings decode to []byte.
// CBOR text strings decode to string.
// CBOR arrays decode to []interface{}.
// CBOR maps decode to map[interface{}]interface{}.
// CBOR null and undefined values decode to nil.
// CBOR times (tag 0 and 1) decode to time.Time.
// CBOR bignums (tag 2 and 3) decode to big.Int.
// CBOR booleans decode to bool.
// CBOR positive integers decode to uint64.
// CBOR negative integers decode to int64 (big.Int if value overflows).
// CBOR floating points decode to float64.
// CBOR byte strings decode to []byte.
// CBOR text strings decode to string.
// CBOR arrays decode to []interface{}.
// CBOR maps decode to map[interface{}]interface{}.
// CBOR null and undefined values decode to nil.
// CBOR times (tag 0 and 1) decode to time.Time.
// CBOR bignums (tag 2 and 3) decode to big.Int.
//
// To unmarshal a CBOR array into a slice, Unmarshal allocates a new slice
// if the CBOR array is empty or slice capacity is less than CBOR array length.
Expand All @@ -75,9 +75,9 @@ import (
// To unmarshal a CBOR map into a struct, Unmarshal matches CBOR map keys to the
// keys in the following priority:
//
// 1. "cbor" key in struct field tag,
// 2. "json" key in struct field tag,
// 3. struct field name.
// 1. "cbor" key in struct field tag,
// 2. "json" key in struct field tag,
// 3. struct field name.
//
// Unmarshal tries an exact match for field name, then a case-insensitive match.
// Map key-value pairs without corresponding struct fields are ignored. See
Expand Down Expand Up @@ -549,7 +549,16 @@ func (dm *decMode) DecOptions() DecOptions {
// See the documentation for Unmarshal for details.
func (dm *decMode) Unmarshal(data []byte, v interface{}) error {
d := decoder{data: data, dm: dm}
return d.value(v, false)

// check valid
off := d.off // Save offset before data validation
err := d.valid(false) // don't allow any extra data after valid data item.
d.off = off // Restore offset
if err != nil {
return err
}

return d.value(v)
}

// Valid checks whether the CBOR data is complete and well-formed.
Expand All @@ -570,10 +579,10 @@ type decoder struct {
}

// value decodes CBOR data item into the value pointed to by v.
// If CBOR data item is invalid, error is returned and offset isn't changed.
// If CBOR data item is valid but fails to be decode into v for other reasons,
// If CBOR data item fails to be decoded into v,
// error is returned and offset is moved to the next CBOR data item.
func (d *decoder) value(v interface{}, allowExtraData bool) error {
// Precondition: d.data contains at least one valid CBOR data item.
func (d *decoder) value(v interface{}) error {
// v can't be nil, non-pointer, or nil pointer value.
if v == nil {
return &InvalidUnmarshalError{"cbor: Unmarshal(nil)"}
Expand All @@ -584,14 +593,6 @@ func (d *decoder) value(v interface{}, allowExtraData bool) error {
} else if rv.IsNil() {
return &InvalidUnmarshalError{"cbor: Unmarshal(nil " + rv.Type().String() + ")"}
}

off := d.off // Save offset before data validation
err := d.valid(allowExtraData)
d.off = off // Restore offset
if err != nil {
return err
}

rv = rv.Elem()
return d.parseToValue(rv, getTypeInfo(rv.Type()))
}
Expand Down
142 changes: 83 additions & 59 deletions stream.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ type Decoder struct {
buf []byte
off int // next read offset in buf
bytesRead int
readError error
}

// NewDecoder returns a new decoder that reads and decodes from r using
Expand All @@ -27,79 +26,105 @@ func NewDecoder(r io.Reader) *Decoder {

// Decode reads CBOR value and decodes it into the value pointed to by v.
func (dec *Decoder) Decode(v interface{}) error {
if len(dec.buf) == dec.off {
if n, err := dec.read(); n == 0 {
return err
}
}
for {
dec.d.reset(dec.buf[dec.off:])
err := dec.d.value(v, true)
// Increment dec.off even if err is not nil because
// dec.d.off points to the next CBOR data item if current
// CBOR data item is valid but failed to be decoded into v.
// This allows next CBOR data item to be decoded in next
// call to this function.
dec.off += dec.d.off
dec.bytesRead += dec.d.off
if err == nil {
return nil
}
if err != io.ErrUnexpectedEOF {
return err
}
// Need to read more data.
if n, err := dec.read(); n == 0 {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return err
}
_, err := dec.readNext()
if err != nil {
// Return validation error or read error.
return err
}

dec.d.reset(dec.buf[dec.off:])
err = dec.d.value(v)

// Increment dec.off even if decoding err is not nil because
// dec.d.off points to the next CBOR data item if current
// CBOR data item is valid but failed to be decoded into v.
// This allows next CBOR data item to be decoded in next
// call to this function.
dec.off += dec.d.off
dec.bytesRead += dec.d.off

return err
}

// Skip skips to the next CBOR data item (if there is any),
// otherwise it returns error such as io.EOF, io.UnexpectedEOF, etc.
func (dec *Decoder) Skip() error {
if len(dec.buf) == dec.off {
if n, err := dec.read(); n == 0 {
return err
}
}
for {
dec.d.reset(dec.buf[dec.off:])
err := dec.d.valid(true)
if err == nil {
// Only increment dec.off if current CBOR data item is valid.
// If current data item is incomplete (io.ErrUnexpectedEOF),
// we want to try again after reading more data.
dec.off += dec.d.off
dec.bytesRead += dec.d.off
return nil
}
if err != io.ErrUnexpectedEOF {
return err
}
// Need to read more data.
if n, err := dec.read(); n == 0 {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return err
}
n, err := dec.readNext()
if err != nil {
// Return validation error or read error.
return err
}

dec.off += n
dec.bytesRead += n
return nil
}

// NumBytesRead returns the number of bytes read.
func (dec *Decoder) NumBytesRead() int {
return dec.bytesRead
}

func (dec *Decoder) read() (int, error) {
if dec.readError != nil {
return 0, dec.readError
// readNext() reads next CBOR data item from Reader to buffer.
// It returns the size of next CBOR data item.
// It also returns validation error or read error if any.
func (dec *Decoder) readNext() (int, error) {
var readErr error
var validErr error

for {
// Process any unread data in dec.buf.
if dec.off < len(dec.buf) {
dec.d.reset(dec.buf[dec.off:])
off := dec.off // Save offset before data validation
validErr = dec.d.valid(true)
dec.off = off // Restore offset

if validErr == nil {
return dec.d.off, nil
}

if validErr != io.ErrUnexpectedEOF {
return 0, validErr
}

// Process last read error on io.ErrUnexpectedEOF.
if readErr != nil {
if readErr == io.EOF {
// current CBOR data item is incomplete.
return 0, io.ErrUnexpectedEOF
}
return 0, readErr
}
}

// More data is needed and there was no read error.
var n int
for n == 0 {
n, readErr = dec.read()
if n == 0 && readErr != nil {
// No more data can be read and read error is encountered.
// At this point, validErr is either nil or io.ErrUnexpectedEOF.
if readErr == io.EOF {
if validErr == io.ErrUnexpectedEOF {
// current CBOR data item is incomplete.
return 0, io.ErrUnexpectedEOF
}
}
return 0, readErr
}
}

// At this point, dec.buf contains new data from last read (n > 0).
}
}

// read() reads data from Reader to buffer.
// It returns number of bytes read and any read error encountered.
// Postconditions:
// - dec.buf contains previously unread data and new data.
// - dec.off is 0.
func (dec *Decoder) read() (int, error) {
// Grow buf if needed.
const minRead = 512
if cap(dec.buf)-len(dec.buf)+dec.off < minRead {
Expand All @@ -116,7 +141,6 @@ func (dec *Decoder) read() (int, error) {
// Read from reader and reslice buf.
n, err := dec.r.Read(dec.buf[len(dec.buf):cap(dec.buf)])
dec.buf = dec.buf[0 : len(dec.buf)+n]
dec.readError = err
return n, err
}

Expand Down
Loading

0 comments on commit f9e6291

Please sign in to comment.