Skip to content

Error instead of truncating over-size packets #11

Description

@rflandau

The protocol package is responsible for interacting with Slim's L5 Orv header. It provides structs and functions for marshaling and unmarshaling these headers and treating any remaining bytes as the body (if appropriate). protocol.ReceivePacket() reads bytes off the wire and forwards them off for deserialization:

go func() { // read the next packet and send it along our channel
	var buf = make([]byte, slims.MaxPacketSize)
	n, senderAddr, err := pconn.ReadFrom(buf)
	if err == nil {
		buf = buf[:n] // trim off excess capacity
	}

	pktCh <- struct {
		n    int
		buf  []byte
		addr net.Addr
		err  error
	}{n, buf, senderAddr, err}
}()

Bytes are read into a buffer of slims.MaxPacketSize (1KB), effectively truncating messages that exceed this length. The Write* and Serialize functions do not check their own length. These two effects combine to cause obscure bugs, typically surfacing because a downstream caller failed to deserialize a message for unknown reasons (the actual reason being that it was unable to deserialize the protobuf, as protobufs require the complete message to be deserialized properly).

This whole situation must be remedied; using the protocol package should not cause obscure bugs elsewhere.

The Solution

protocol should be reworked to:

  1. Not silently truncate when receiving. This likely means returning an error or sending an error along an error channel for the caller to handle.
  2. Length check messages when writing them

Ultimately, I did not write protocol defensively and should have.

The Caveat

protocol undergirds all of Slims and could realistically see use outside of the Slims parent package. These changes should limit breaking changes to the API lest they create substantial additional work within slims or create other, silent errors.

Metadata

Metadata

Assignees

No one assigned

    Labels

    SlimsIssue related to Orv SlimsbugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions