Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect against stray EOFs #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Protect against stray EOFs #44

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25f41c9
Protect against stray EOFs
Stebalien Aug 27, 2020
3907992
cleanup: use a helper function for ErrUnexpectedEOF
Stebalien Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions gen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,9 @@ func emitCborUnmarshalStructField(w io.Writer, f Field) error {
if extra > 0 {
buf := make([]byte, extra)
if _, err := io.ReadFull(br, buf); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return err
}
{{ .Name }} = big.NewInt(0).SetBytes(buf)
Expand Down Expand Up @@ -898,6 +901,9 @@ func emitCborUnmarshalSliceField(w io.Writer, f Field) error {
}
{{end}}
if _, err := io.ReadFull(br, {{ .Name }}[:]); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return err
}
`)
Expand Down
8 changes: 8 additions & 0 deletions peeker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,16 @@ func (p *peeker) ReadByte() (byte, error) {
var buf [1]byte
n, err := p.reader.Read(buf[:])
if n == 0 {
if err == nil {
err = io.ErrNoProgress
}
return 0, err
}
// ReadByte is not allowed to return an EOF when a byte was successfully
// read, but the underlying reader is allowed to do so.
if err == io.EOF {
err = nil
}
b := buf[0]
p.lastByte = b
p.peekState = peekSet
Expand Down
12 changes: 12 additions & 0 deletions testing/cbor_gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions testing/cbor_map_gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

72 changes: 61 additions & 11 deletions utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,29 @@ const maxHeaderSize = 9
// the most common readers we encounter in this library for a significant
// performance boost.
func discard(br io.Reader, n int) error {
var err error
switch r := br.(type) {
case *bytes.Buffer:
buf := r.Next(n)
if len(buf) < n {
return io.ErrUnexpectedEOF
err = io.ErrUnexpectedEOF
}
return nil
case *bytes.Reader:
if r.Len() < n {
_, _ = r.Seek(0, io.SeekEnd)
return io.ErrUnexpectedEOF
err = io.ErrUnexpectedEOF
} else {
_, err = r.Seek(int64(n), io.SeekCurrent)
}
_, err := r.Seek(int64(n), io.SeekCurrent)
return err
case *bufio.Reader:
_, err := r.Discard(n)
return err
_, err = r.Discard(n)
default:
_, err := io.CopyN(ioutil.Discard, br, int64(n))
return err
_, err = io.CopyN(ioutil.Discard, br, int64(n))
}
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return err
}

func ScanForLinks(br io.Reader, cb func(cid.Cid)) error {
Expand Down Expand Up @@ -74,6 +76,10 @@ func ScanForLinks(br io.Reader, cb func(cid.Cid)) error {
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without knowing this code it is hard to know which functions are safe from returning io.EOFs, for example CborReadHeaderBuf above does not check for EOFs but from the names and arguments it seems like it could return them. I don't know if this is actually what we should do but here and in discard it would help me be more sure EOF handling is correct if we did error escalation in a defer statement.


if _, err := io.ReadAtLeast(br, scratch[:extra], int(extra)); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}

return err
}

Expand Down Expand Up @@ -172,7 +178,7 @@ func (d *Deferred) UnmarshalCBOR(br io.Reader) error {
// Copy the bytes
limitedReader.N = int64(extra)
buf.Grow(int(extra))
if n, err := buf.ReadFrom(&limitedReader); err != nil {
if n, err := buf.ReadFrom(&limitedReader); err != nil && err != io.EOF {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buf.ReadFrom doesn't return io.EOF: https://pkg.go.dev/[email protected]#Buffer.ReadFrom

return err
} else if n < int64(extra) {
return io.ErrUnexpectedEOF
Expand Down Expand Up @@ -214,6 +220,9 @@ func readByte(r io.Reader) (byte, error) {
}
var buf [1]byte
_, err := io.ReadFull(r, buf[:1])
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return buf[0], err
}

Expand Down Expand Up @@ -241,6 +250,9 @@ func CborReadHeader(br io.Reader) (byte, uint64, error) {
case low == 25:
scratch := make([]byte, 2)
if _, err := io.ReadAtLeast(br, scratch[:2], 2); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return 0, 0, err
}
val := uint64(binary.BigEndian.Uint16(scratch[:2]))
Expand All @@ -251,6 +263,9 @@ func CborReadHeader(br io.Reader) (byte, uint64, error) {
case low == 26:
scratch := make([]byte, 4)
if _, err := io.ReadAtLeast(br, scratch[:4], 4); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return 0, 0, err
}
val := uint64(binary.BigEndian.Uint32(scratch[:4]))
Expand All @@ -261,6 +276,9 @@ func CborReadHeader(br io.Reader) (byte, uint64, error) {
case low == 27:
scratch := make([]byte, 8)
if _, err := io.ReadAtLeast(br, scratch, 8); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return 0, 0, err
}
val := binary.BigEndian.Uint64(scratch)
Expand All @@ -276,6 +294,7 @@ func CborReadHeader(br io.Reader) (byte, uint64, error) {
func readByteBuf(r io.Reader, scratch []byte) (byte, error) {
// Reading a single byte from these buffers is much faster than copying
// into a slice.
// None of these will return EOF if they also read a byte.
switch r := r.(type) {
case *bytes.Buffer:
return r.ReadByte()
Expand All @@ -285,13 +304,23 @@ func readByteBuf(r io.Reader, scratch []byte) (byte, error) {
return r.ReadByte()
}
n, err := r.Read(scratch[:1])
if err == io.EOF {
// Read can return EOF, even if it reads data. Furthermore, we
// never want to return EOF on actual error cases (so we return
// "unexpected eof").
if n == 1 {
err = nil
} else {
err = io.ErrUnexpectedEOF
}
}
if err != nil {
return 0, err
}
if n != 1 {
return 0, fmt.Errorf("failed to read a byte")
}
return scratch[0], err
return scratch[0], nil
}

// same as the above, just tries to allocate less by using a passed in scratch buffer
Expand All @@ -318,6 +347,9 @@ func CborReadHeaderBuf(br io.Reader, scratch []byte) (byte, uint64, error) {
return maj, uint64(next), nil
case low == 25:
if _, err := io.ReadAtLeast(br, scratch[:2], 2); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return 0, 0, err
}
val := uint64(binary.BigEndian.Uint16(scratch[:2]))
Expand All @@ -327,15 +359,24 @@ func CborReadHeaderBuf(br io.Reader, scratch []byte) (byte, uint64, error) {
return maj, val, nil
case low == 26:
if _, err := io.ReadAtLeast(br, scratch[:4], 4); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return 0, 0, err
}
val := uint64(binary.BigEndian.Uint32(scratch[:4]))
if val <= math.MaxUint16 {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return 0, 0, fmt.Errorf("cbor input was not canonical (lval 26 with value <= MaxUint16)")
}
return maj, val, nil
case low == 27:
if _, err := io.ReadAtLeast(br, scratch[:8], 8); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return 0, 0, err
}
val := binary.BigEndian.Uint64(scratch[:8])
Expand Down Expand Up @@ -475,6 +516,9 @@ func ReadByteArray(br io.Reader, maxlen uint64) ([]byte, error) {

buf := make([]byte, extra)
if _, err := io.ReadAtLeast(br, buf, int(extra)); err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return nil, err
}

Expand Down Expand Up @@ -516,6 +560,9 @@ func ReadString(r io.Reader) (string, error) {
buf := make([]byte, l)
_, err = io.ReadAtLeast(r, buf, int(l))
if err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return "", err
}

Expand All @@ -539,6 +586,9 @@ func ReadStringBuf(r io.Reader, scratch []byte) (string, error) {
buf := make([]byte, l)
_, err = io.ReadAtLeast(r, buf, int(l))
if err != nil {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return "", err
}

Expand Down