Repository: bmatsuo/lmdb-go (c)

Issue:
Exception with cursor.Get(k, nil, Set)
I have db with dup values (not sure if relevant). When I'm trying to do ```k, v, e = cursor.Get(buf.Bytes(), nil, lmdb.Set)``` I get exception (see below). With ```k, v, e = cursor.Get(buf.Bytes(), nil, lmdb.SetKey)``` it works. Just Set positions cursor at the key but doesn't read value. But cursor.Get still attempts to read it which leads to the exception.

```
panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 21 [running]:
panic(0x56f360, 0xc0420b0260)
        C:/Go/src/runtime/panic.go:500 +0x1af
github.com/bmatsuo/lmdb-go/lmdb._cgoCheckPointer0(0xc0420bc024, 0x0, 0x0, 0x0, 0x0)
        ??:0 +0x60
github.com/bmatsuo/lmdb-go/lmdb.getBytesCopy(0xc0420b0170, 0xc0420b0170, 0x23190e0, 0x556ee0)
        D:/Projects/Go/src/github.com/bmatsuo/lmdb-go/lmdb/val.go:120 +0x51
github.com/bmatsuo/lmdb-go/lmdb.(*Txn).bytes(0xc0420b6120, 0xc0420b0170, 0x25, 0x40, 0xf)
        D:/Projects/Go/src/github.com/bmatsuo/lmdb-go/lmdb/txn.go:276 +0x7a
github.com/bmatsuo/lmdb-go/lmdb.(*Cursor).Get(0xc0420b01a0, 0xc0420bc024, 0x25, 0x40, 0x0, 0x0, 0x0, 0xf, 0xa0, 0x61, ...)
        D:/Projects/Go/src/github.com/bmatsuo/lmdb-go/lmdb/cursor.go:155 +0x147
github.com/bmatsuo/lmdb-go/lmdbscan.(*Scanner).Set(0xc0420ba120, 0xc0420bc024, 0x25, 0x40, 0x0, 0x0, 0x0, 0xf, 0x4c)
        D:/Projects/Go/src/github.com/bmatsuo/lmdb-go/lmdbscan/scanner.go:74 +0xc4
github.com/bmatsuo/lmdb-go/lmdbscan.(*Scanner).SetNext(0xc0420ba120, 0xc0420bc024, 0x25, 0x40, 0x0, 0x0, 0x0, 0xf, 0x9, 0x25)
        D:/Projects/Go/src/github.com/bmatsuo/lmdb-go/lmdbscan/scanner.go:85 +0x9b
bitbucket.org/agileharbor/lmdbbenchmark/lmdbLayer.(*Reader).ReadDupRecords.func1(0xc0420b6120, 0x0, 0x0)
        D:/Projects/Go/src/bitbucket.org/agileharbor/lmdbbenchmark/lmdbLayer/reader.go:103 +0x83d
github.com/bmatsuo/lmdb-go/lmdb.(*Env).run(0xc042072040, 0x412c00, 0x20000, 0xc0420ae0f0, 0x0, 0x0)
        D:/Projects/Go/src/github.com/bmatsuo/lmdb-go/lmdb/env.go:461 +0x157
github.com/bmatsuo/lmdb-go/lmdb.(*Env).View(0xc042072040, 0xc0420ae0f0, 0x9, 0xc000000002)
        D:/Projects/Go/src/github.com/bmatsuo/lmdb-go/lmdb/env.go:423 +0x4a
```

Pull request:
Cursor.Get handles key return values specicially for Set
Fixes #96

Issue #56 illustrated that working with Go memory using "C-style" memory
does not work well in modern Go runtimes.  That problem was thought to
be properly addressed, but #96 showed a lingering case where Set does
not modify MDB_val key passed to mdb_cursor_get.

This commit addresses this lingering case by special-casing return value
processing for the Set operation of Cursor.Get.  If LMDB will not modify
the MDB_val, the returned key must use non-standard methods for copying
the value.  Likewise, when txn.RawRead is true the Cursor.Get must use a
special method to ensure that the returned key shares memory with the
corresponding input argument.

Patch:
diff --git a/lmdb/cursor.go b/lmdb/cursor.go
index b82caea..b9c89db 100644
--- a/lmdb/cursor.go
+++ b/lmdb/cursor.go
@@ -135,6 +135,11 @@ func (c *Cursor) DBI() DBI {
 // returned by Get reference readonly sections of memory that must not be
 // accessed after the transaction has terminated.
 //
+// In a Txn with RawRead set to true the Set op causes the returned key to
+// share its memory with setkey (making it writable memory). In a Txn with
+// RawRead set to false the Set op returns key values with memory distinct from
+// setkey, as is always the case when using RawRead.
+//
 // Get ignores setval if setkey is empty.
 //
 // See mdb_cursor_get.
@@ -152,11 +157,30 @@ func (c *Cursor) Get(setkey, setval []byte, op uint) (key, val []byte, err error
 		*c.txn.val = C.MDB_val{}
 		return nil, nil, err
 	}
-	k := c.txn.bytes(c.txn.key)
-	v := c.txn.bytes(c.txn.val)
+
+	// When MDB_SET is passed to mdb_cursor_get its first argument will be
+	// returned unchanged.  Unfortunately, the normal slice copy/extraction
+	// routines will be bad for the Go runtime when operating on Go memory
+	// (panic or potentially garbage memory reference).
+	if op == Set {
+		if c.txn.RawRead {
+			key = setkey
+		} else {
+			p := make([]byte, len(setkey))
+			copy(p, setkey)
+			key = p
+		}
+	} else {
+		key = c.txn.bytes(c.txn.key)
+	}
+	val = c.txn.bytes(c.txn.val)
+
+	// Clear transaction storage record storage area for future use and to
+	// prevent dangling references.
 	*c.txn.key = C.MDB_val{}
 	*c.txn.val = C.MDB_val{}
-	return k, v, nil
+
+	return key, val, nil
 }
 
 // getVal0 retrieves items from the database without using given key or value


Test patch:
diff --git a/lmdb/cursor_test.go b/lmdb/cursor_test.go
index 5537726..3514c6d 100644
--- a/lmdb/cursor_test.go
+++ b/lmdb/cursor_test.go
@@ -250,7 +250,7 @@ func TestCursor_Get_KV(t *testing.T) {
 			t.Errorf("unexpected key: %q (not %q)", k, "key")
 		}
 		if string(v) != "1" {
-			t.Errorf("unexpected key: %q (not %q)", k, "1")
+			t.Errorf("unexpected value: %q (not %q)", k, "1")
 		}
 
 		_, _, err = cur.Get([]byte("key"), []byte("1"), GetBoth)
@@ -261,6 +261,82 @@ func TestCursor_Get_KV(t *testing.T) {
 	}
 }
 
+func TestCursor_Get_op_Set_bytesBuffer(t *testing.T) {
+	env := setup(t)
+	defer clean(env, t)
+
+	var dbi DBI
+	err := env.Update(func(txn *Txn) (err error) {
+		dbi, err = txn.OpenDBI("testdb", Create|DupSort)
+		return err
+	})
+	if err != nil {
+		t.Errorf("%s", err)
+		return
+	}
+
+	err = env.Update(func(txn *Txn) (err error) {
+		put := func(k, v []byte) {
+			if err == nil {
+				err = txn.Put(dbi, k, v, 0)
+			}
+		}
+		put([]byte("k1"), []byte("v11"))
+		put([]byte("k1"), []byte("v12"))
+		put([]byte("k1"), []byte("v13"))
+		put([]byte("k2"), []byte("v21"))
+		put([]byte("k2"), []byte("v22"))
+		put([]byte("k2"), []byte("v23"))
+		return err
+	})
+	if err != nil {
+		t.Errorf("%s", err)
+	}
+
+	err = env.View(func(txn *Txn) (err error) {
+		cur, err := txn.OpenCursor(dbi)
+		if err != nil {
+			return err
+		}
+		defer cur.Close()
+
+		// Create bytes.Buffer values containing a amount of bytes.  Byte
+		// slices returned from buf.Bytes() have a history of tricking the cgo
+		// argument checker.
+		var kbuf bytes.Buffer
+		kbuf.WriteString("k2")
+
+		k, _, err := cur.Get(kbuf.Bytes(), nil, Set)
+		if err != nil {
+			return err
+		}
+		if string(k) != kbuf.String() {
+			t.Errorf("unexpected key: %q (not %q)", k, kbuf.String())
+		}
+
+		// No guarantee is made about the return value of mdb_cursor_get when
+		// MDB_SET is the op, so its value is not checked as part of this test.
+		// That said, it is important that Cursor.Get not panic if given a
+		// short buffer as an input value for a Set op (despite that not really
+		// having any significance)
+		var vbuf bytes.Buffer
+		vbuf.WriteString("v22")
+
+		k, _, err = cur.Get(kbuf.Bytes(), vbuf.Bytes(), Set)
+		if err != nil {
+			return err
+		}
+		if string(k) != kbuf.String() {
+			t.Errorf("unexpected key: %q (not %q)", k, kbuf.String())
+		}
+
+		return nil
+	})
+	if err != nil {
+		t.Errorf("%s", err)
+	}
+}
+
 func TestCursor_Get_DupFixed(t *testing.T) {
 	env := setup(t)
 	defer clean(env, t)