Fix Index out of Range in QueryPlayers() (#10)

* Squashed commit of the following:

commit 6ba9f84
Author: Marcin Pawlowski <[email protected]>
Date:   Mon Dec 12 16:13:14 2022 -0800

    cleanup some errors

commit 49bc058
Author: Marcin Pawlowski <[email protected]>
Date:   Mon Dec 12 16:07:53 2022 -0800

    clean up errors

commit ba93af6
Author: Marcin Pawlowski <[email protected]>
Date:   Mon Dec 12 15:43:26 2022 -0800

    fix player-info panics

* update go to 1.19 in build

* build fuzz test

.gitignore

@@ -7,6 +7,7 @@
 
 # Test binary, build with `go test -c`
 *.test
+testdata/**
 
 # Output of the go coverage tool, specifically when used with LiteIDE
 *.out

.travis.yml

@@ -1,7 +1,7 @@
 language: go
 
 go:
-  - 1.15
+  - 1.19
   - tip
 
 os:
@@ -15,3 +15,4 @@ matrix:
 
 script:
   - go test
+  - go test -run FuzzParsePlayerInfo -fuzz FuzzParsePlayerInfo -fuzztime 1m

client.go

@@ -20,6 +20,7 @@ var (
 
 type Client struct {
 	addr          string
+	shouldConnect bool
 	conn          net.Conn
 	timeout       time.Duration
 	maxPacketSize uint32
@@ -54,6 +55,14 @@ func SetAppID(appid int32) func(*Client) error {
 	}
 }
 
+// disableDial can disable dialing the given address on client creation to prevent UDP floods in unit tests.
+func disableDial() func(*Client) error {
+	return func(c *Client) error {
+		c.shouldConnect = false
+		return nil
+	}
+}
+
 // SetMaxPacketSize changes the maximum buffer size of a UDP packet
 // Note that some games such as squad may use a non-standard packet size
 // Refer to the game documentation to see if this needs to be changed
@@ -68,6 +77,7 @@ func SetMaxPacketSize(size uint32) func(*Client) error {
 func NewClient(addr string, options ...func(*Client) error) (c *Client, err error) {
 	c = &Client{
 		timeout:       DefaultTimeout,
+		shouldConnect: true,
 		addr:          addr,
 		maxPacketSize: DefaultMaxPacketSize,
 	}
@@ -85,8 +95,10 @@ func NewClient(addr string, options ...func(*Client) error) (c *Client, err erro
 		c.addr = fmt.Sprintf("%s:%d", c.addr, DefaultPort)
 	}
 
-	if c.conn, err = net.DialTimeout("udp", c.addr, c.timeout); err != nil {
-		return nil, err
+	if c.shouldConnect {
+		if c.conn, err = net.DialTimeout("udp", c.addr, c.timeout); err != nil {
+			return nil, err
+		}
 	}
 
 	c.buffer = make([]byte, 0, c.maxPacketSize)
@@ -129,6 +141,9 @@ func (c *Client) receive() ([]byte, error) {
 }
 
 func (c *Client) Close() error {
+	if !c.shouldConnect {
+		return nil
+	}
 	return c.conn.Close()
 }
 

go.mod

@@ -0,0 +1,11 @@
+module github.com/rumblefrog/go-a2s
+
+go 1.19
+
+require github.com/stretchr/testify v1.8.1
+
+require (
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
+)

go.sum

@@ -0,0 +1,17 @@
+github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
+github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
+github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
+github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk=
+github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

info_test.go

@@ -2,23 +2,20 @@ package a2s
 
 import (
 	"encoding/json"
+	"flag"
 	"fmt"
 	"testing"
 )
 
-const (
-	TestHost = "s1.zhenyangli.me"
-)
+var testHost = flag.String("test-host", "s1.zhenyangli.me", "Remote hostname to use for unit tests.")
 
 func TestInfo(t *testing.T) {
-	c, err := NewClient(TestHost)
-
-	defer c.Close()
-
+	c, err := NewClient(*testHost)
 	if err != nil {
 		t.Error(err)
 		return
 	}
+	defer c.Close()
 
 	i, err := c.QueryInfo()
 

packet.go

@@ -79,6 +79,13 @@ func (r *PacketReader) ReadUint8() uint8 {
 	return b
 }
 
+func (r *PacketReader) TryReadUint8() (uint8, bool) {
+	if r.CanRead(1) != nil {
+		return 0, false
+	}
+	return r.ReadUint8(), true
+}
+
 func (r *PacketReader) ReadUint16() uint16 {
 	u16 := binary.LittleEndian.Uint16(r.buffer[r.pos:])
 	r.pos += 2
@@ -91,10 +98,24 @@ func (r *PacketReader) ReadUint32() uint32 {
 	return u32
 }
 
+func (r *PacketReader) TryReadUint32() (uint32, bool) {
+	if r.CanRead(4) != nil {
+		return 0, false
+	}
+	return r.ReadUint32(), true
+}
+
 func (r *PacketReader) ReadInt32() int32 {
 	return int32(r.ReadUint32())
 }
 
+func (r *PacketReader) TryReadInt32() (int32, bool) {
+	if r.CanRead(4) != nil {
+		return 0, false
+	}
+	return r.ReadInt32(), true
+}
+
 func (r *PacketReader) ReadUint64() uint64 {
 	u64 := binary.LittleEndian.Uint64(r.buffer[r.pos:])
 	r.pos += 8
@@ -107,6 +128,13 @@ func (r *PacketReader) ReadFloat32() float32 {
 	return math.Float32frombits(bits)
 }
 
+func (r *PacketReader) TryReadFloat32() (float32, bool) {
+	if r.CanRead(4) != nil {
+		return 0, false
+	}
+	return r.ReadFloat32(), true
+}
+
 func (r *PacketReader) TryReadString() (string, bool) {
 	start := r.pos
 	for r.pos < len(r.buffer) {

player.go

@@ -110,28 +110,50 @@ func (c *Client) parsePlayerInfo(data []byte) (*PlayerInfo, error) {
 	reader := NewPacketReader(data)
 
 	// Simple response now
-
-	if reader.ReadInt32() != -1 {
-		return nil, ErrBadPacketHeader
+	_, ok := reader.TryReadInt32()
+	if !ok {
+		return nil, ErrBadPlayerReply
 	}
 
-	if reader.ReadUint8() != A2S_PLAYER_RESPONSE {
+	headerByte, ok := reader.TryReadUint8()
+	if !ok || headerByte != A2S_PLAYER_RESPONSE {
 		return nil, ErrBadPlayerReply
 	}
 
 	info := &PlayerInfo{}
 
-	info.Count = reader.ReadUint8()
+	count, hasCount := reader.TryReadUint8()
+	if !hasCount {
+		return nil, ErrBadPlayerReply
+	}
+
+	info.Count = count
 
 	var player *Player
 
 	for i := 0; i < int(info.Count); i++ {
 		player = &Player{}
 
-		player.Index = reader.ReadUint8()
-		player.Name = reader.ReadString()
-		player.Score = reader.ReadUint32()
-		player.Duration = reader.ReadFloat32()
+		index, hasIndex := reader.TryReadUint8()
+		if !hasIndex {
+			return nil, ErrBadPlayerReply
+		}
+		player.Index = index
+		name, hasName := reader.TryReadString()
+		if !hasName {
+			return nil, ErrBadPlayerReply
+		}
+		player.Name = name
+		score, hasScore := reader.TryReadUint32()
+		if !hasScore {
+			return nil, ErrBadPlayerReply
+		}
+		player.Score = score
+		duration, hasDuration := reader.TryReadFloat32()
+		if !hasDuration {
+			return nil, ErrBadPlayerReply
+		}
+		player.Duration = duration
 
 		/*
 			The Ship additional player info
@@ -141,8 +163,17 @@ func (c *Client) parsePlayerInfo(data []byte) (*PlayerInfo, error) {
 		if c.appid == App_TheShip {
 			player.TheShip = &TheShipPlayer{}
 
-			player.TheShip.Deaths = reader.ReadUint32()
-			player.TheShip.Money = reader.ReadUint32()
+			shipDeaths, hasShipDeaths := reader.TryReadUint32()
+			if !hasShipDeaths {
+				return nil, ErrBadPlayerReply
+			}
+			player.TheShip.Deaths = shipDeaths
+
+			shipMoney, hasShipMoney := reader.TryReadUint32()
+			if !hasShipMoney {
+				return nil, ErrBadPlayerReply
+			}
+			player.TheShip.Money = shipMoney
 		}
 
 		info.Players = append(info.Players, player)

player_test.go

@@ -4,17 +4,17 @@ import (
 	"encoding/json"
 	"fmt"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestPlayer(t *testing.T) {
-	c, err := NewClient(TestHost)
-
-	defer c.Close()
-
+	c, err := NewClient(*testHost)
 	if err != nil {
 		t.Error(err)
 		return
 	}
+	defer c.Close()
 
 	p, err := c.QueryPlayer()
 
@@ -27,3 +27,57 @@ func TestPlayer(t *testing.T) {
 
 	fmt.Println(string(JSON))
 }
+
+// Example response from https://developer.valvesoftware.com/wiki/Server_queries#Response_Format_2
+func validPlayerInfoPacket() []byte {
+	return []byte{
+		0xFF, 0xFF, 0xFF, 0xFF, 0x44, 0x02, 0x01, 0x5B, 0x44, 0x5D, 0x2D, 0x2D, 0x2D, 0x2D, 0x3E, 0x54,
+		0x2E, 0x4E, 0x2E, 0x57, 0x3C, 0x2D, 0x2D, 0x2D, 0x2D, 0x00, 0x0E, 0x00, 0x00, 0x00, 0xB4, 0x97,
+		0x00, 0x44, 0x02, 0x4B, 0x69, 0x6C, 0x6C, 0x65, 0x72, 0x20, 0x21, 0x21, 0x21, 0x00, 0x05, 0x00,
+		0x00, 0x00, 0x69, 0x24, 0xD9, 0x43,
+	}
+}
+
+func TestParsePlayerInfo(t *testing.T) {
+	c, err := NewClient(*testHost, disableDial())
+	assert.Nil(t, err, "NewClient should not return an error")
+	defer c.Close()
+
+	info, err := c.parsePlayerInfo(validPlayerInfoPacket())
+	assert.Nil(t, err, "player info should not fail for a valid packet")
+	assert.Equal(t, uint8(0x2), info.Count, "Player count should match")
+	assert.Equal(t, 2, len(info.Players), "Player count should match actual number of players parsed")
+	assert.Equal(t, uint8(1), info.Players[0].Index, "Player index should match")
+	assert.Equal(t, "[D]---->T.N.W<----", info.Players[0].Name, "Player name should match")
+	assert.Equal(t, float32(514.37036), info.Players[0].Duration, "Player duration should match")
+	assert.Equal(t, uint32(14), info.Players[0].Score, "Player score should match")
+	assert.Equal(t, uint8(2), info.Players[1].Index, "Player index should match")
+	assert.Equal(t, "Killer !!!", info.Players[1].Name, "Player name should match")
+	assert.Equal(t, float32(434.28445), info.Players[1].Duration, "Player duration should match")
+	assert.Equal(t, uint32(5), info.Players[1].Score, "Player score should match")
+}
+
+func FuzzParsePlayerInfo(f *testing.F) {
+
+	validPacket := validPlayerInfoPacket()
+
+	// seed corpus from a valid packet
+	for i := 0; i < len(validPacket); i++ {
+		f.Add(validPacket[i:], int32(0))
+		f.Add(validPacket[:i], int32(0))
+		f.Add(validPacket[i:], int32(App_TheShip))
+		f.Add(validPacket[:i], int32(App_TheShip))
+	}
+
+	f.Fuzz(func(t *testing.T, a []byte, appId int32) {
+		c, err := NewClient(*testHost, SetAppID(appId), disableDial())
+		assert.Nil(f, err, "NewClient should not return an error")
+		defer c.Close()
+		_, err = c.parsePlayerInfo(a)
+		// sometimes the fuzzer can actually generate valid player info, if so, skip since we are only checking for panics when an invalid packet is returned from a server
+		if err == nil {
+			t.Skip()
+		}
+		assert.NotNil(t, err, "invalid parsePlayerInfo fuzzing should return an error")
+	})
+}

rules_test.go

@@ -7,7 +7,7 @@ import (
 )
 
 func TestRules(t *testing.T) {
-	c, err := NewClient(TestHost)
+	c, err := NewClient(*testHost)
 
 	defer c.Close()