New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

database/sql: all named []byte types get driver memory like RawBytes #13905

Closed
jmoiron opened this Issue Jan 11, 2016 · 4 comments

Comments

Projects
None yet
4 participants
@jmoiron
Copy link

jmoiron commented Jan 11, 2016

  • Go version: 1.4.3, 1.5.2, and git
  • OS: Linux x86_64

I added this test to src/database/sql/convert_test.go:

func TestUserDefinedBytes(t *testing.T) {
    type userDefinedBytes []byte
    var u userDefinedBytes
    v := []byte("foo")

    convertAssign(&u, v)
    if &u[0] == &v[0] {
        t.Fatal("userDefinedBytes got potentially dirty driver memory")
    }
}
  • I expected this test to pass, but
  • It fails

The problem is line 207 of convert.go; in this case, dest is assignable the src pointer, so it just assigns it instead of cloning it. The documentation doesn't actually make any promises about where memory is going to come from when you call Scan, but it seems obvious from the code that RawBytes was supposed to be treated specially as it gets driver memory via a different code path. Having that behavior inadvertently for non-RawBytes named []byte types seems like an oversight.

This patch fixes it, though it may potentially be present for other types or in line 217.

diff --git a/src/database/sql/convert.go b/src/database/sql/convert.go
index bba5a88..fda6293 100644
--- a/src/database/sql/convert.go
+++ b/src/database/sql/convert.go
@@ -204,7 +204,12 @@ func convertAssign(dest, src interface{}) error {

        dv := reflect.Indirect(dpv)
        if sv.IsValid() && sv.Type().AssignableTo(dv.Type()) {
-               dv.Set(sv)
+               switch b := src.(type) {
+               case []byte:
+                       dv.Set(reflect.ValueOf(cloneBytes(b)))
+               default:
+                       dv.Set(sv)
+               }
                return nil
        }

I signed the CLA and wouldn't mind going through the contribution process, though I've never done so.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 11, 2016

@gofish

This comment has been minimized.

Copy link

gofish commented Jan 14, 2016

A temporary workaround may be an explicit cast to *[]byte.

This passes the test:

convertAssign((*[]byte)(&u),v)

For json.RawMessage:

var msg json.RawMessage
row := db.QueryRow(`SELECT '{}'::json`)
err := row.Scan((*[]byte)(&msg))
...
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 25, 2016

This is not a regression, and we are late in the release cycle, so postponing until 1.7.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Apr 23, 2016

CL https://golang.org/cl/22393 mentions this issue.

@gopherbot gopherbot closed this in 4e0cd1e Apr 30, 2016

@golang golang locked and limited conversation to collaborators Apr 30, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.