Commit a5094338 authored by Kirill Smelkov's avatar Kirill Smelkov

encoder: More unexpected EOF handling

Continuing 995fce81 I've decided to rework unexpected EOF testing to run
on truncated versions of all inputs we already have. This uncovered the
following:

- if an individual op decoder returns err=io.EOF - this is unexpected
  and should be turned into io.ErrUnexpectedEOF. If we do not do it
  shows as e.g.

        ogorek_test.go:91: int: no ErrUnexpectedEOF on [:1] truncated stream: v = <nil>  err = &errors.errorString{s:"EOF"}

- there was a potential buffer overrun panic in loadString - the code
  there did not have proper check for line rest length after initial ".
  The panic looks like this:

--- FAIL: TestDecode (0.00s)
panic: runtime error: slice bounds out of range [recovered]
        panic: runtime error: slice bounds out of range

goroutine 7 [running]:
testing.tRunner.func1(0xc4200725b0)
        /home/kirr/src/tools/go/go/src/testing/testing.go:624 +0x267
panic(0x5368c0, 0x603020)
        /home/kirr/src/tools/go/go/src/runtime/panic.go:489 +0x26e
github.com/kisielk/og-rek.(*Decoder).loadString(0xc420063428, 0xc42000e153, 0x0)
        /home/kirr/src/wendelin/neo/g.neo/src/github.com/kisielk/og-rek/ogorek.go:478 +0x22d
github.com/kisielk/og-rek.Decoder.Decode(0xc42012fbc0, 0x6248e8, 0x0, 0x0, 0xc42013ac60, 0x0, 0x0, 0x5f2480, 0xc42000e150)
        /home/kirr/src/wendelin/neo/g.neo/src/github.com/kisielk/og-rek/ogorek.go:166 +0xa83
github.com/kisielk/og-rek.TestDecode(0xc4200725b0)
        /home/kirr/src/wendelin/neo/g.neo/src/github.com/kisielk/og-rek/ogorek_test.go:89 +0x26c7
testing.tRunner(0xc4200725b0, 0x567618)
        /home/kirr/src/tools/go/go/src/testing/testing.go:659 +0x98
created by testing.(*T).Run
        /home/kirr/src/tools/go/go/src/testing/testing.go:701 +0x2df
exit status 2

  and can be triggered by single-character " input.

- we also turn what was previously an "insecure string" into unexpected
  EOF. The reason here is: code could not find " in the end of the
  string. This can be for 2 reasons:

  1. EOF was reached, or
  2. EOL (end of line) was reached

  unfortunately since we are currently using bufio.Buffer.ReadLine()
  which does not indicate whether EOL was there or not and strips it
  from result, we cannot distinguish above cases and properly report
  "insecure string" in 2 case.

  However unexpected EOF even for #2 seems a bit reasonable, so I think
  the change should be ok.
parent d266ce2f
...@@ -246,6 +246,10 @@ loop: ...@@ -246,6 +246,10 @@ loop:
if err == errNotImplemented { if err == errNotImplemented {
return nil, OpcodeError{key, insn} return nil, OpcodeError{key, insn}
} }
// EOF from individual opcode decoder is unexpected end of stream
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
return nil, err return nil, err
} }
} }
...@@ -468,8 +472,8 @@ func (d *Decoder) loadString() error { ...@@ -468,8 +472,8 @@ func (d *Decoder) loadString() error {
return fmt.Errorf("invalid string delimiter: %c", line[0]) return fmt.Errorf("invalid string delimiter: %c", line[0])
} }
if line[len(line)-1] != delim { if len(line) < 2 || line[len(line)-1] != delim {
return fmt.Errorf("insecure string") return io.ErrUnexpectedEOF
} }
d.push(decodeStringEscape(line[1 : len(line)-1])) d.push(decodeStringEscape(line[1 : len(line)-1]))
......
...@@ -6,6 +6,8 @@ import ( ...@@ -6,6 +6,8 @@ import (
"io" "io"
"math/big" "math/big"
"reflect" "reflect"
"strconv"
"strings"
"testing" "testing"
) )
...@@ -81,6 +83,23 @@ func TestDecode(t *testing.T) { ...@@ -81,6 +83,23 @@ func TestDecode(t *testing.T) {
if !(v == nil && err == io.EOF) { if !(v == nil && err == io.EOF) {
t.Errorf("decode: no EOF at end: v = %#v err = %#v", v, err) t.Errorf("decode: no EOF at end: v = %#v err = %#v", v, err)
} }
// for truncated input io.ErrUnexpectedEOF must be returned
for l := len(test.input) - 1; l > 0; l-- {
buf := bytes.NewBufferString(test.input[:l])
dec := NewDecoder(buf)
//println(test.name, l)
v, err := dec.Decode()
// strconv.UnquoteChar used in loadUnicode always returns
// SyntaxError, at least unless the following CL is accepted:
// https://go-review.googlesource.com/37052
if err == strconv.ErrSyntax && strings.HasPrefix(test.name, "unicode") {
err = io.ErrUnexpectedEOF
}
if !(v == nil && err == io.ErrUnexpectedEOF) {
t.Errorf("%s: no ErrUnexpectedEOF on [:%d] truncated stream: v = %#v err = %#v", test.name, l, v, err)
}
}
} }
} }
...@@ -110,17 +129,6 @@ func TestDecodeMultiple(t *testing.T) { ...@@ -110,17 +129,6 @@ func TestDecodeMultiple(t *testing.T) {
} }
} }
func TestDecodeUnexpectedEOF(t *testing.T) {
input := "I5\n"
buf := bytes.NewBufferString(input)
dec := NewDecoder(buf)
obj, err := dec.Decode()
if !(obj == nil && err == io.ErrUnexpectedEOF) {
t.Errorf("decode: no ErrUnexpectedEOF on truncated stream: obj = %#v err = %#v", obj, err)
}
}
func TestZeroLengthData(t *testing.T) { func TestZeroLengthData(t *testing.T) {
data := "" data := ""
output, err := decodeLong(data) output, err := decodeLong(data)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment