Repository: gocql/gocql
Pull request #1450 (2020-05-27 14:50:09)

fix tuple unmarshaling
There are two problems.

First, tuples with a null value are not handled properly which results in a
panic:

    panic: runtime error: slice bounds out of range [:-1]

    goroutine 1 [running]:
    github.com/gocql/gocql.unmarshalTuple(0x769880, 0xc0000e8cc0, 0xc0000ae83a, 0x10, 0x4a, 0xc000186540, 0xc000190480, 0xc000190480, 0x16)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:1950 +0x1276
    github.com/gocql/gocql.Unmarshal(0x769880, 0xc0000e8cc0, 0xc0000ae836, 0x10, 0x4a, 0xc000186540, 0xc000190480, 0xc0000e8cc0, 0xc0000ae832)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:167 +0xd56
    github.com/gocql/gocql.unmarshalMap(0x769800, 0xc0000e8d00, 0xc0000ae836, 0x10, 0x4e, 0x698740, 0xc000190450, 0x0, 0x677f11)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:1669 +0x8fb
    github.com/gocql/gocql.Unmarshal(0x769800, 0xc0000e8d00, 0xc0000ae828, 0x1e, 0x58, 0x698740, 0xc000190450, 0x0, 0x0)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:159 +0xcbf
    github.com/gocql/gocql.scanColumn(0xc0000ae828, 0x1e, 0x58, 0xc0000b0988, 0x6, 0xc0000b09a0, 0xc, 0xc0000b09b0, 0x4, 0x769800, ...)
            /home/vincent/dev/go/src/github.com/gocql/gocql/session.go:1314 +0x274
    github.com/gocql/gocql.(*Iter).Scan(0xc0001963f0, 0xc000190460, 0x2, 0x2, 0x1)
            /home/vincent/dev/go/src/github.com/gocql/gocql/session.go:1414 +0x2e5
    main.main()
            /home/vincent/tmp/gocql-tuple-map/main.go:90 +0x9cc

It panics when unmarshaling into a struct or into an array/slice.

Second, the unmarshaling code does not work with pointer fields in a
struct or pointer elements in an array/slice which results in a panic:

    panic: reflect.Set: value of type time.Time is not assignable to type *time.Time

    goroutine 1 [running]:
    reflect.Value.assignTo(0x6fa700, 0xc000282280, 0x199, 0x7026c8, 0xb, 0x6fc0e0, 0x0, 0x0, 0x6fc0e0, 0xc000282280)
            /usr/local/go/src/reflect/value.go:2403 +0x426
    reflect.Value.Set(0x6fc0e0, 0xc0002821b8, 0x196, 0x6fa700, 0xc000282280, 0x199)
            /usr/local/go/src/reflect/value.go:1532 +0xbd
    github.com/gocql/gocql.unmarshalTuple(0x7698c0, 0xc000384100, 0xc0003aa0b6, 0x10, 0x4a, 0xc00028c1c0, 0xc0002821a0, 0xc0002821a0, 0x16)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:1952 +0xf06
    github.com/gocql/gocql.Unmarshal(0x7698c0, 0xc000384100, 0xc0003aa0b6, 0x10, 0x4a, 0xc00028c1c0, 0xc0002821a0, 0xc000384100, 0xc0003aa0b2)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:166 +0xd56
    github.com/gocql/gocql.unmarshalMap(0x769840, 0xc000384140, 0xc0003aa0b6, 0x10, 0x4e, 0x698720, 0xc000282170, 0x0, 0x677e51)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:1668 +0x8fb
    github.com/gocql/gocql.Unmarshal(0x769840, 0xc000384140, 0xc0003aa0a8, 0x1e, 0x58, 0x698720, 0xc000282170, 0x0, 0x0)
            /home/vincent/dev/go/src/github.com/gocql/gocql/marshal.go:158 +0xcbf
    github.com/gocql/gocql.scanColumn(0xc0003aa0a8, 0x1e, 0x58, 0xc000382018, 0x6, 0xc000382030, 0xc, 0xc000382040, 0x4, 0x769840, ...)
            /home/vincent/dev/go/src/github.com/gocql/gocql/session.go:1314 +0x274
    github.com/gocql/gocql.(*Iter).Scan(0xc00029a120, 0xc000282180, 0x2, 0x2, 0x1)
            /home/vincent/dev/go/src/github.com/gocql/gocql/session.go:1414 +0x2e5
    main.main()
            /home/vincent/tmp/gocql-tuple-map/main.go:90 +0x9cc

This commit fixes both problems.

You can reproduce the panics with the following code:
```
package main

import (
	"flag"
	"log"
	"time"

	"github.com/gocql/gocql"
)

func main() {
	var (
		flNilDate          = flag.Bool("nil-date", false, "use a nil date")
		flUnmarshalInArray = flag.Bool("unmarshal-in-array", false, "unmarshal in an array")
	)

	flag.Parse()

	const schema = `CREATE TABLE IF NOT EXISTS foobar.map_of_tuple (id uuid PRIMARY KEY, data map<smallint, frozen<tuple<timestamp, timestamp>>>)`

	cfg := gocql.NewCluster("localhost:9042")

	session, err := cfg.CreateSession()
	if err != nil {
		log.Fatal(err)
	}

	err = session.Query(schema).Exec()
	if err != nil {
		log.Fatal(err)
	}

	id, _ := gocql.RandomUUID()

	switch {
	case *flNilDate:
		now := time.Now().Add(-2000 * time.Hour)
		myTuple := struct {
			ID   time.Time
			Date *time.Time
		}{now, nil}

		err = session.Query(`update foobar.map_of_tuple set data[?] = (?, ?) where id = ?`, 20, myTuple.ID, myTuple.Date, id).Exec()
		if err != nil {
			log.Fatal(err)
		}

	default:
		before := time.Now().Add(-2000 * time.Hour)
		now := time.Now()

		myTuple := struct {
			ID   time.Time
			Date *time.Time
		}{before, &now}

		err = session.Query(`update foobar.map_of_tuple set data[?] = (?, ?) where id = ?`, 20, myTuple.ID, myTuple.Date, id).Exec()
		if err != nil {
			log.Fatal(err)
		}
	}

	//

	iter := session.Query(`select id, data from foobar.map_of_tuple where id = ?`, id).Iter()

	switch {
	case *flUnmarshalInArray:
		var row struct {
			id   gocql.UUID
			data map[int][2]*time.Time
		}

		iter.Scan(&row.id, &row.data)
		if err := iter.Close(); err != nil {
			log.Fatal(err)
		}

		log.Printf("id: %v data: %+v", id, row.data)

	default:
		var row struct {
			id   gocql.UUID
			data map[int]struct {
				ID   time.Time
				Date *time.Time
			}
		}

		iter.Scan(&row.id, &row.data)
		if err := iter.Close(); err != nil {
			log.Fatal(err)
		}

		log.Printf("id: %v data: %+v", id, row.data)
	}

}
```

vrischmann: I'll take a look
vrischmann: I've added tests which should cover all added code.

Patch:
diff --git a/conn.go b/conn.go
index 578af47..9e349c2 100644
--- a/conn.go
+++ b/conn.go
@@ -213,7 +213,6 @@ func (s *Session) dialWithoutObserver(ctx context.Context, host *HostInfo, cfg *
 		dialer = d
 	}
 
-
 	conn, err := dialer.DialContext(ctx, "tcp", host.HostnameAndPort())
 	if err != nil {
 		return nil, err
diff --git a/marshal.go b/marshal.go
index fda5be6..dd1d5e6 100644
--- a/marshal.go
+++ b/marshal.go
@@ -1951,16 +1951,22 @@ func unmarshalTuple(info TypeInfo, data []byte, value interface{}) error {
 		}
 
 		for i, elem := range tuple.Elems {
-			m := readInt(data)
-			data = data[4:]
+			var p []byte
+			if len(data) > 4 {
+				p, data = readBytes(data)
+			}
 
 			v := elem.New()
-			if err := Unmarshal(elem, data[:m], v); err != nil {
+			if err := Unmarshal(elem, p, v); err != nil {
 				return err
 			}
-			rv.Field(i).Set(reflect.ValueOf(v).Elem())
 
-			data = data[m:]
+			switch rv.Field(i).Kind() {
+			case reflect.Ptr:
+				rv.Field(i).Set(reflect.ValueOf(v))
+			default:
+				rv.Field(i).Set(reflect.ValueOf(v).Elem())
+			}
 		}
 
 		return nil
@@ -1975,16 +1981,22 @@ func unmarshalTuple(info TypeInfo, data []byte, value interface{}) error {
 		}
 
 		for i, elem := range tuple.Elems {
-			m := readInt(data)
-			data = data[4:]
+			var p []byte
+			if len(data) > 4 {
+				p, data = readBytes(data)
+			}
 
 			v := elem.New()
-			if err := Unmarshal(elem, data[:m], v); err != nil {
+			if err := Unmarshal(elem, p, v); err != nil {
 				return err
 			}
-			rv.Index(i).Set(reflect.ValueOf(v).Elem())
 
-			data = data[m:]
+			switch rv.Index(i).Kind() {
+			case reflect.Ptr:
+				rv.Index(i).Set(reflect.ValueOf(v))
+			default:
+				rv.Index(i).Set(reflect.ValueOf(v).Elem())
+			}
 		}
 
 		return nil
diff --git a/session.go b/session.go
index 6c7edff..c627e4b 100644
--- a/session.go
+++ b/session.go
@@ -655,7 +655,7 @@ func (s *Session) ExecuteBatch(batch *Batch) error {
 }
 
 // ExecuteBatchCAS executes a batch operation and returns true if successful and
-// an iterator (to scan aditional rows if more than one conditional statement)
+// an iterator (to scan additional rows if more than one conditional statement)
 // was sent.
 // Further scans on the interator must also remember to include
 // the applied boolean as the first argument to *Iter.Scan
@@ -698,7 +698,7 @@ func (s *Session) MapExecuteBatchCAS(batch *Batch, dest map[string]interface{})
 type hostMetrics struct {
 	// Attempts is count of how many times this query has been attempted for this host.
 	// An attempt is either a retry or fetching next page of results.
-	Attempts     int
+	Attempts int
 
 	// TotalLatency is the sum of attempt latencies for this host in nanoseconds.
 	TotalLatency int64
@@ -872,7 +872,7 @@ func (q *Query) Latency() int64 {
 }
 
 func (q *Query) AddLatency(l int64, host *HostInfo) {
-	q.metrics.attempt(0, time.Duration(l) * time.Nanosecond, host, false)
+	q.metrics.attempt(0, time.Duration(l)*time.Nanosecond, host, false)
 }
 
 // Consistency sets the consistency level for this query. If no consistency
@@ -944,7 +944,7 @@ func (q *Query) DefaultTimestamp(enable bool) *Query {
 // WithTimestamp will enable the with default timestamp flag on the query
 // like DefaultTimestamp does. But also allows to define value for timestamp.
 // It works the same way as USING TIMESTAMP in the query itself, but
-// should not break prepared query optimization
+// should not break prepared query optimization.
 //
 // Only available on protocol >= 3
 func (q *Query) WithTimestamp(timestamp int64) *Query {
@@ -1135,7 +1135,7 @@ func (q *Query) PageState(state []byte) *Query {
 // NoSkipMetadata will override the internal result metadata cache so that the driver does not
 // send skip_metadata for queries, this means that the result will always contain
 // the metadata to parse the rows and will not reuse the metadata from the prepared
-// staement. This should only be used to work around cassandra bugs, such as when using
+// statement. This should only be used to work around cassandra bugs, such as when using
 // CAS operations which do not end in Cas.
 //
 // See https://issues.apache.org/jira/browse/CASSANDRA-11099
@@ -1608,7 +1608,7 @@ func (b *Batch) Latency() int64 {
 }
 
 func (b *Batch) AddLatency(l int64, host *HostInfo) {
-	b.metrics.attempt(0, time.Duration(l) * time.Nanosecond, host, false)
+	b.metrics.attempt(0, time.Duration(l)*time.Nanosecond, host, false)
 }
 
 // GetConsistency returns the currently configured consistency level for the batch
@@ -1722,7 +1722,7 @@ func (b *Batch) DefaultTimestamp(enable bool) *Batch {
 // WithTimestamp will enable the with default timestamp flag on the query
 // like DefaultTimestamp does. But also allows to define value for timestamp.
 // It works the same way as USING TIMESTAMP in the query itself, but
-// should not break prepared query optimization
+// should not break prepared query optimization.
 //
 // Only available on protocol >= 3
 func (b *Batch) WithTimestamp(timestamp int64) *Batch {


Test patch:
diff --git a/marshal_test.go b/marshal_test.go
index 5160850..5809d3e 100644
--- a/marshal_test.go
+++ b/marshal_test.go
@@ -1736,6 +1736,81 @@ func TestMarshalTuple(t *testing.T) {
 	}
 }
 
+func TestUnmarshalTuple(t *testing.T) {
+	info := TupleTypeInfo{
+		NativeType: NativeType{proto: 3, typ: TypeTuple},
+		Elems: []TypeInfo{
+			NativeType{proto: 3, typ: TypeVarchar},
+			NativeType{proto: 3, typ: TypeVarchar},
+		},
+	}
+
+	// As per the CQL spec, a tuple is a sequence of "bytes" values.
+	// Here we encode a null value (length -1) and the "foo" string (length 3)
+
+	data := []byte("\xff\xff\xff\xff\x00\x00\x00\x03foo")
+
+	t.Run("struct-ptr", func(t *testing.T) {
+		var tmp struct {
+			A *string
+			B *string
+		}
+
+		err := Unmarshal(info, data, &tmp)
+		if err != nil {
+			t.Errorf("unmarshalTest: %v", err)
+			return
+		}
+
+		if *tmp.A != "" || *tmp.B != "foo" {
+			t.Errorf("unmarshalTest: expected [nil, foo], got [%v, %v]", *tmp.A, *tmp.B)
+		}
+	})
+	t.Run("struct-nonptr", func(t *testing.T) {
+		var tmp struct {
+			A string
+			B string
+		}
+
+		err := Unmarshal(info, data, &tmp)
+		if err != nil {
+			t.Errorf("unmarshalTest: %v", err)
+			return
+		}
+
+		if tmp.A != "" || tmp.B != "foo" {
+			t.Errorf("unmarshalTest: expected [nil, foo], got [%v, %v]", tmp.A, tmp.B)
+		}
+	})
+
+	t.Run("array", func(t *testing.T) {
+		var tmp [2]*string
+
+		err := Unmarshal(info, data, &tmp)
+		if err != nil {
+			t.Errorf("unmarshalTest: %v", err)
+			return
+		}
+
+		if *tmp[0] != "" || *tmp[1] != "foo" {
+			t.Errorf("unmarshalTest: expected [nil, foo], got [%v, %v]", *tmp[0], *tmp[1])
+		}
+	})
+	t.Run("array-nonptr", func(t *testing.T) {
+		var tmp [2]string
+
+		err := Unmarshal(info, data, &tmp)
+		if err != nil {
+			t.Errorf("unmarshalTest: %v", err)
+			return
+		}
+
+		if tmp[0] != "" || tmp[1] != "foo" {
+			t.Errorf("unmarshalTest: expected [nil, foo], got [%v, %v]", tmp[0], tmp[1])
+		}
+	})
+}
+
 func TestMarshalNil(t *testing.T) {
 	types := []Type{
 		TypeAscii,