op-node: Fix Frame parsing

Fixes CLI-3355 (Sherlock #279)

op-node/rollup/derive/frame.go

@@ -68,30 +68,41 @@ type ByteReader interface {
 // UnmarshalBinary consumes a full frame from the reader.
 // If `r` fails a read, it returns the error from the reader
 // The reader will be left in a partially read state.
+//
+// If r doesn't return any bytes, returns io.EOF.
+// If r unexpectedly stops returning data half-way, returns io.ErrUnexpectedEOF.
 func (f *Frame) UnmarshalBinary(r ByteReader) error {
 	if _, err := io.ReadFull(r, f.ID[:]); err != nil {
 		return fmt.Errorf("error reading ID: %w", err)
 	}
-	if err := binary.Read(r, binary.BigEndian, &f.FrameNumber); err != nil {
-		return fmt.Errorf("error reading frame number: %w", err)
+	if err := binary.Read(r, binary.BigEndian, &f.FrameNumber); err == io.EOF {
+		return fmt.Errorf("frame_number missing: %w", io.ErrUnexpectedEOF)
+	} else if err != nil {
+		return fmt.Errorf("reading frame_number: %w", err)
 	}
 
 	var frameLength uint32
-	if err := binary.Read(r, binary.BigEndian, &frameLength); err != nil {
-		return fmt.Errorf("error reading frame length: %w", err)
+	if err := binary.Read(r, binary.BigEndian, &frameLength); err == io.EOF {
+		return fmt.Errorf("frame_data_length missing: %w", io.ErrUnexpectedEOF)
+	} else if err != nil {
+		return fmt.Errorf("reading frame_data_length: %w", err)
 	}
 
 	// Cap frame length to MaxFrameLen (currently 1MB)
 	if frameLength > MaxFrameLen {
-		return fmt.Errorf("frameLength is too large: %d", frameLength)
+		return fmt.Errorf("frame_data_length is too large: %d", frameLength)
 	}
 	f.Data = make([]byte, int(frameLength))
-	if _, err := io.ReadFull(r, f.Data); err != nil {
-		return fmt.Errorf("error reading frame data: %w", err)
+	if _, err := io.ReadFull(r, f.Data); err == io.EOF {
+		return fmt.Errorf("frame_data missing: %w", io.ErrUnexpectedEOF)
+	} else if err != nil {
+		return fmt.Errorf("reading frame_data: %w", err)
 	}
 
-	if isLastByte, err := r.ReadByte(); err != nil && err != io.EOF {
-		return fmt.Errorf("error reading final byte: %w", err)
+	if isLastByte, err := r.ReadByte(); err == io.EOF {
+		return fmt.Errorf("final byte (is_last) missing: %w", io.ErrUnexpectedEOF)
+	} else if err != nil {
+		return fmt.Errorf("reading final byte (is_last): %w", err)
 	} else if isLastByte == 0 {
 		f.IsLast = false
 		return err
@@ -123,8 +134,8 @@ func ParseFrames(data []byte) ([]Frame, error) {
 	var frames []Frame
 	for buf.Len() > 0 {
 		var f Frame
-		if err := (&f).UnmarshalBinary(buf); err != io.EOF && err != nil {
-			return nil, err
+		if err := f.UnmarshalBinary(buf); err != nil {
+			return nil, fmt.Errorf("parsing frame %d: %w", len(frames), err)
 		}
 		frames = append(frames, f)
 	}

op-node/rollup/derive/frame_test.go

@@ -2,7 +2,15 @@ package derive
 
 import (
 	"bytes"
+	"io"
+	"math"
+	"math/rand"
+	"strconv"
 	"testing"
+	"time"
+
+	"github.com/ethereum-optimism/optimism/op-node/testutils"
+	"github.com/stretchr/testify/require"
 )
 
 func FuzzFrameUnmarshalBinary(f *testing.F) {
@@ -23,3 +31,122 @@ func FuzzParseFrames(f *testing.F) {
 		}
 	})
 }
+
+func TestFrameMarshaling(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	for i := 0; i < 16; i++ {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			frame := randomFrame(rng)
+			var data bytes.Buffer
+			require.NoError(t, frame.MarshalBinary(&data))
+
+			frame0 := new(Frame)
+			require.NoError(t, frame0.UnmarshalBinary(&data))
+			require.Equal(t, frame, frame0)
+		})
+	}
+}
+
+func TestFrameUnmarshalTruncated(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+
+	for _, tr := range []struct {
+		desc     string
+		truncate func([]byte) []byte
+	}{
+		{
+			desc: "truncate-frame_data_length-full",
+			truncate: func(data []byte) []byte {
+				return data[:18] // truncate full frame_data_length
+			},
+		},
+		{
+			desc: "truncate-frame_data_length-half",
+			truncate: func(data []byte) []byte {
+				return data[:20] // truncate half-way frame_data_length
+			},
+		},
+		{
+			desc: "truncate-data-full",
+			truncate: func(data []byte) []byte {
+				return data[:22] // truncate after frame_data_length
+			},
+		},
+		{
+			desc: "truncate-data-last-byte",
+			truncate: func(data []byte) []byte {
+				return data[:len(data)-2]
+			},
+		},
+		{
+			desc: "truncate-is_last",
+			truncate: func(data []byte) []byte {
+				return data[:len(data)-1]
+			},
+		},
+	} {
+		t.Run(tr.desc, func(t *testing.T) {
+			frame := randomFrame(rng)
+			var data bytes.Buffer
+			require.NoError(t, frame.MarshalBinary(&data))
+
+			// truncate last data byte & is_last
+			tdata := tr.truncate(data.Bytes())
+
+			frame0 := new(Frame)
+			err := frame0.UnmarshalBinary(bytes.NewReader(tdata))
+			require.Error(t, err)
+			require.ErrorIs(t, err, io.ErrUnexpectedEOF)
+		})
+	}
+}
+
+func TestFrameUnmarshalInvalidIsLast(t *testing.T) {
+	rng := rand.New(rand.NewSource(time.Now().UnixNano()))
+	frame := randomFrame(rng, frameWithDataLen(16))
+	var data bytes.Buffer
+	require.NoError(t, frame.MarshalBinary(&data))
+
+	idata := data.Bytes()
+	idata[len(idata)-1] = 2 // invalid is_last
+
+	frame0 := new(Frame)
+	err := frame0.UnmarshalBinary(bytes.NewReader(idata))
+	require.Error(t, err)
+	require.ErrorContains(t, err, "invalid byte")
+}
+
+func randomFrame(rng *rand.Rand, opts ...frameOpt) *Frame {
+	var id ChannelID
+	_, err := rng.Read(id[:])
+	if err != nil {
+		panic(err)
+	}
+
+	frame := &Frame{
+		ID:          id,
+		FrameNumber: uint16(rng.Int31n(math.MaxUint16 + 1)),
+		IsLast:      testutils.RandomBool(rng),
+	}
+
+	// evaulaute options
+	for _, opt := range opts {
+		opt(rng, frame)
+	}
+
+	// default if no option set field
+	if frame.Data == nil {
+		datalen := int(rng.Intn(MaxFrameLen + 1))
+		frame.Data = testutils.RandomData(rng, datalen)
+	}
+
+	return frame
+}
+
+type frameOpt func(*rand.Rand, *Frame)
+
+func frameWithDataLen(l int) frameOpt {
+	return func(rng *rand.Rand, frame *Frame) {
+		frame.Data = testutils.RandomData(rng, l)
+	}
+}