mysql icon indicating copy to clipboard operation
mysql copied to clipboard

handleOkPacket panic: runtime error: slice bounds out of range

Open shuaichang opened this issue 6 years ago • 5 comments

Issue description

Encountered panic, seems len(data) can be less then 1 but was not checked?

// Ok Packet
// http://dev.mysql.com/doc/internals/en/generic-response-packets.html#packet-OK_Packet
func (mc *mysqlConn) handleOkPacket(data []byte) error {
	var n, m int

	// 0x00 [1 byte]

	// Affected rows [Length Coded Binary]
	mc.affectedRows, _, n = readLengthEncodedInteger(data[1:])

	// Insert id [Length Coded Binary]
	mc.insertId, _, m = readLengthEncodedInteger(data[1+n:])

	// server_status [2 bytes]
	mc.status = readStatus(data[1+n+m : 1+n+m+2])
	if err := mc.discardResults(); err != nil {
		return err
	}

	// warning count [2 bytes]
	if !mc.strict {
		return nil
	}

	pos := 1 + n + m + 2
	if binary.LittleEndian.Uint16(data[pos:pos+2]) > 0 {
		return mc.getWarnings()
	}
	return nil
}
panic: runtime error: slice bounds out of range
goroutine 132643 [running]:
aliyun/serverless/admin-service/vendor/github.com/go-sql-driver/mysql.(*mysqlConn).handleOkPacket(0xc420a4f290, 0xc4206fc004, 0x1, 0xffc, 0x0, 0x0)
    /go/src/aliyun/serverless/admin-service/vendor/github.com/go-sql-driver/mysql/packets.go:583 +0x230
aliyun/serverless/admin-service/vendor/github.com/go-sql-driver/mysql.(*mysqlConn).readResultSetHeaderPacket(0xc420a4f290, 0x3, 0xc420196000, 0x2d0)

Example code

		rowsX, err := w.db.QueryContext(ctx, querySQL)
		if err != nil {
			w.logger.Errorf("Failed to make query %s due to %v", queryName, err)
			return err
		}

Error log

If you have an error log, please paste it here.

Configuration

*Driver version (or git SHA): version: commit a0583e0143b1624142adab07e0e97fe106d99561

Go version: run go version in your console go version go1.8 linux/amd64

Server version: E.g. MySQL 5.6, MariaDB 10.0.20 Not a Mysql SQL, only compatible to make mysql queries

Server OS: E.g. Debian 8.1 (Jessie), Windows 10 NA

shuaichang avatar Oct 22 '18 19:10 shuaichang

Encountered panic, seems len(data) can be less then 1 but was not checked?

Panic is a bug, but len(data) can not be less than 1.

What database do you use? It doesn't implement MySQL protocol correctly. We will return ErrMalfoldPacket instead of panic. So you can not use this driver for your database anyway.

methane avatar Oct 23 '18 00:10 methane

In which version/commit the fix was merged in?

shuaichang avatar Nov 22 '18 02:11 shuaichang

Probably none, but we still didn't receive enough details to reproduce this panic.

julienschmidt avatar Nov 22 '18 11:11 julienschmidt

@shuaichang Do you really want this bug is fixed? Most people using server talking valid protocol are not affected by this bug. Few people using server talking invalid protocol (including you) can't use this driver even if this bug is fixed. You just get ErrMalfoldPacket instead of panic.

Fixing this bug is not joyful task...

methane avatar Nov 24 '18 05:11 methane

@shuaichang Of course, if you want to fix this bug, I'll review your PR :)

It's boring, but not hard. (1) Write if len(data) < ... { return ErrMalfoldPacket } everywhere needed. (2) Write tests until all new codes are covered.

methane avatar Nov 24 '18 05:11 methane