database/sql: sql.RawBytes is not compatible with contexts

[bradfitz]

sql.RawBytes says:

RawBytes is a byte slice that holds a reference to memory owned by the database itself. After a Scan into a RawBytes, the slice is only valid until the next call to Next, Scan, or Close.

That was true when RawBytes was added (in the first Go release, Go 1(.0)), but Go 1.8 added context.Context support to the database/sql package.

Since that time, RawBytes has been unsafe to use with any BeginTx/QueryContext with a context that might be done, as internally the database/sql package starts a goroutine (awaitDone) waiting for the context(s) to be done and then calls Close on the database/sql/driver.Conn, concurrently with the caller working with RawBytes.

Consider this series of events:

      ctx, cancel := context.WithCancel(context.Background())
      rows, ... := QueryRowContext(ctx)
      for rows.Next() {
           var raw sql.RawBytes
           rows.Scan(&raw)
           cancel()   // imagine this a timeout or cancellation
           json.Unmarshal(raw, ....)  // working with memory that's not ours
     }

Options:

• document that RawBytes can't be used with a context that might be Done
• make convertAssignRows clone the driver's []byte memory when a vulnerable context is in use
• change the goroutine that does the concurrent driver.Close to instead set an atomic bool that the context is done, and do the Close+error on the next call to Rows.Next/Rows.Close/Rows.Err/etc.... something that the user did explicitly, rather than doing something concurrently with the user code working with the RawBytes.

What version of Go are you using (go version)?

Go 1.20, but happens starting with Go 1.8 probably (not verified)

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

macOS, Linux, etc. Not platform-specific.

[rsc]

Looks like this is a duplicate of #53970, although we didn't realize the severity of that one when it came in.

[rsc]

Closed #53970 as a duplicate of this one, because this one is the better presentation.

[rsc]

Of the three options, the atomic bool and delayed driver close seems like the right fix. /cc @kardianos

[rsc]

@gopherbot please backport Go 1.20 and Go 1.19

[gopherbot]

Backport issue(s) opened: #60307 (for 1.19), #60308 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[renthraysk]

The MySQL driver solution was to switch buffers if an error/cancellation happened.

go-sql-driver/mysql#943

[heschi]

@rsc can you explain why this is a release blocker for 1.21? #53970 was filed nearly a year ago, and the problem has supposedly existed since 1.8. This seems like a bug worth fixing, but do we really need to hold 1.21 for it?

[rsc]

Even though it's old, it causes data corruption in code that looks perfectly innocent (and arguably is innocent).
I believe @bradfitz is working on a fix. If at all possible we really should put the fix in the release and backport it.

[gopherbot]

Change https://go.dev/cl/497675 mentions this issue: database/sql: fix use of Rows with context + RawBytes

[heschi]

@bradfitz seems like the CL above fixed this? Should we close?

[gopherbot]

Change https://go.dev/cl/498398 mentions this issue: database/sql: fix regression from earlier RawBytes fix

[ianlancetaylor]

Is there anything else left to do for this issue?

[rsc]

All set.