sql: fix potential corruption in QueryRow.Scan into a *[]byte

Fixes #2622

R=golang-dev, adg
CC=golang-dev
https://golang.org/cl/5533077

diff --git a/src/pkg/exp/sql/fakedb_test.go b/src/pkg/exp/sql/fakedb_test.go
index 0883dd9f3edaa5707a52e169a180f8489503d1d7..0a1dd091e3086415c540b3d097e59f2d10d1537f 100644 (file)
--- a/src/pkg/exp/sql/fakedb_test.go
+++ b/src/pkg/exp/sql/fakedb_test.go
@@ -306,6 +306,8 @@ func (c *fakeConn) prepareInsert(stmt *fakeStmt, parts []string) (driver.Stmt, e
                        switch ctype {
                        case "string":
                                subsetVal = []byte(value)
+                       case "blob":
+                               subsetVal = []byte(value)
                        case "int32":
                                i, err := strconv.Atoi(value)
                                if err != nil {
@@ -510,9 +512,19 @@ type rowsCursor struct {
        pos    int
        rows   []*row
        closed bool
+
+       // a clone of slices to give out to clients, indexed by the
+       // the original slice's first byte address.  we clone them
+       // just so we're able to corrupt them on close.
+       bytesClone map[*byte][]byte
 }
 
 func (rc *rowsCursor) Close() error {
+       if !rc.closed {
+               for _, bs := range rc.bytesClone {
+                       bs[0] = 255 // first byte corrupted
+               }
+       }
        rc.closed = true
        return nil
 }
@@ -537,6 +549,19 @@ func (rc *rowsCursor) Next(dest []interface{}) error {
                // for ease of drivers, and to prevent drivers from
                // messing up conversions or doing them differently.
                dest[i] = v
+
+               if bs, ok := v.([]byte); ok {
+                       if rc.bytesClone == nil {
+                               rc.bytesClone = make(map[*byte][]byte)
+                       }
+                       clone, ok := rc.bytesClone[&bs[0]]
+                       if !ok {
+                               clone = make([]byte, len(bs))
+                               copy(clone, bs)
+                               rc.bytesClone[&bs[0]] = clone
+                       }
+                       dest[i] = clone
+               }
        }
        return nil
 }

diff --git a/src/pkg/exp/sql/sql.go b/src/pkg/exp/sql/sql.go
index f53691b7c42dad179d5c1dbf333e7e40cb6c7a68..a076fcdcbc2f6722b3a4293c5492d9c097910b96 100644 (file)
--- a/src/pkg/exp/sql/sql.go
+++ b/src/pkg/exp/sql/sql.go
@@ -803,10 +803,6 @@ type Row struct {
 // pointed at by dest.  If more than one row matches the query,
 // Scan uses the first row and discards the rest.  If no row matches
 // the query, Scan returns ErrNoRows.
-//
-// If dest contains pointers to []byte, the slices should not be
-// modified and should only be considered valid until the next call to
-// Next or Scan.
 func (r *Row) Scan(dest ...interface{}) error {
        if r.err != nil {
                return r.err
@@ -815,7 +811,33 @@ func (r *Row) Scan(dest ...interface{}) error {
        if !r.rows.Next() {
                return ErrNoRows
        }
-       return r.rows.Scan(dest...)
+       err := r.rows.Scan(dest...)
+       if err != nil {
+               return err
+       }
+
+       // TODO(bradfitz): for now we need to defensively clone all
+       // []byte that the driver returned, since we're about to close
+       // the Rows in our defer, when we return from this function.
+       // the contract with the driver.Next(...) interface is that it
+       // can return slices into read-only temporary memory that's
+       // only valid until the next Scan/Close.  But the TODO is that
+       // for a lot of drivers, this copy will be unnecessary.  We
+       // should provide an optional interface for drivers to
+       // implement to say, "don't worry, the []bytes that I return
+       // from Next will not be modified again." (for instance, if
+       // they were obtained from the network anyway) But for now we
+       // don't care.
+       for _, dp := range dest {
+               b, ok := dp.(*[]byte)
+               if !ok {
+                       continue
+               }
+               clone := make([]byte, len(*b))
+               copy(clone, *b)
+               *b = clone
+       }
+       return nil
 }
 
 // A Result summarizes an executed SQL command.

diff --git a/src/pkg/exp/sql/sql_test.go b/src/pkg/exp/sql/sql_test.go
index 590bf818fef3ee614de797d14ab6bdfe3eab43b2..77245db96fdab1b6a3cd52759a4620f786323000 100644 (file)
--- a/src/pkg/exp/sql/sql_test.go
+++ b/src/pkg/exp/sql/sql_test.go
@@ -21,10 +21,10 @@ func newTestDB(t *testing.T, name string) *DB {
                t.Fatalf("exec wipe: %v", err)
        }
        if name == "people" {
-               exec(t, db, "CREATE|people|name=string,age=int32,dead=bool")
-               exec(t, db, "INSERT|people|name=Alice,age=?", 1)
-               exec(t, db, "INSERT|people|name=Bob,age=?", 2)
-               exec(t, db, "INSERT|people|name=Chris,age=?", 3)
+               exec(t, db, "CREATE|people|name=string,age=int32,photo=blob,dead=bool")
+               exec(t, db, "INSERT|people|name=Alice,age=?,photo=APHOTO", 1)
+               exec(t, db, "INSERT|people|name=Bob,age=?,photo=BPHOTO", 2)
+               exec(t, db, "INSERT|people|name=Chris,age=?,photo=CPHOTO", 3)
        }
        return db
 }
@@ -132,6 +132,16 @@ func TestQueryRow(t *testing.T) {
        if age != 1 {
                t.Errorf("expected age 1, got %d", age)
        }
+
+       var photo []byte
+       err = db.QueryRow("SELECT|people|photo|name=?", "Alice").Scan(&photo)
+       if err != nil {
+               t.Fatalf("photo QueryRow+Scan: %v", err)
+       }
+       want := []byte("APHOTO")
+       if !reflect.DeepEqual(photo, want) {
+               t.Errorf("photo = %q; want %q", photo, want)
+       }
 }
 
 func TestStatementErrorAfterClose(t *testing.T) {