Skip to content
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 · Fixed by jmoiron/sqlx#247
Closed

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

jmoiron opened this issue Jan 11, 2016 · 4 comments · Fixed by jmoiron/sqlx#247
Milestone

Comments

@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
Copy link
Contributor

CC @bradfitz

@gofish
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
Copy link
Contributor

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

@gopherbot
Copy link

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

@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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants