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

bradfitz · 2023-05-19T16:45:25Z

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.

The text was updated successfully, but these errors were encountered:

rsc · 2023-05-19T16:58:13Z

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

rsc · 2023-05-19T17:00:00Z

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

rsc · 2023-05-19T17:06:32Z

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

rsc · 2023-05-19T17:06:46Z

@gopherbot please backport Go 1.20 and Go 1.19

gopherbot · 2023-05-19T17:06:59Z

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 · 2023-05-19T23:06:30Z

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

go-sql-driver/mysql#943

heschi · 2023-05-22T16:22:33Z

@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 · 2023-05-22T16:50:41Z

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 · 2023-05-23T22:23:11Z

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

sql.RawBytes was added the very first Go release, Go 1. Its docs say: > 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 "only valid until the next call" bit was true at the time, until contexts were added to database/sql in Go 1.8. In the past ~dozen releases it's been unsafe to use QueryContext with a context that might become Done to get an *sql.Rows that's scanning into a RawBytes. The Scan can succeed, but then while the caller's reading the memory, a database/sql-managed goroutine can see the context becoming done and call Close on the database/sql/driver and make the caller's view of the RawBytes memory no longer valid, introducing races, crashes, or database corruption. See #60304 and #53970 for details. This change does the minimal surgery on database/sql to make it safe again: Rows.Scan was already acquiring a mutex to check whether the rows had been closed, so this change make Rows.Scan notice whether *RawBytes was used and, if so, doesn't release the mutex on exit before returning. That mean it's still locked while the user code operates on the RawBytes memory and the concurrent context-watching goroutine to close the database still runs, but if it fires, it then gets blocked on the mutex until the next call to a Rows method (Next, NextResultSet, Err, Close). Updates #60304 Updates #53970 (earlier one I'd missed) Change-Id: Ie41c0c6f32c24887b2f53ec3686c2aab73a1bfff Reviewed-on: https://go-review.googlesource.com/c/go/+/497675 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Russ Cox <rsc@golang.org>

heschi · 2023-05-24T16:08:44Z

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

sql.RawBytes was added the very first Go release, Go 1. Its docs say: > 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 "only valid until the next call" bit was true at the time, until contexts were added to database/sql in Go 1.8. In the past ~dozen releases it's been unsafe to use QueryContext with a context that might become Done to get an *sql.Rows that's scanning into a RawBytes. The Scan can succeed, but then while the caller's reading the memory, a database/sql-managed goroutine can see the context becoming done and call Close on the database/sql/driver and make the caller's view of the RawBytes memory no longer valid, introducing races, crashes, or database corruption. See golang#60304 and golang#53970 for details. This change does the minimal surgery on database/sql to make it safe again: Rows.Scan was already acquiring a mutex to check whether the rows had been closed, so this change make Rows.Scan notice whether *RawBytes was used and, if so, doesn't release the mutex on exit before returning. That mean it's still locked while the user code operates on the RawBytes memory and the concurrent context-watching goroutine to close the database still runs, but if it fires, it then gets blocked on the mutex until the next call to a Rows method (Next, NextResultSet, Err, Close). Updates golang#60304 Updates golang#53970 (earlier one I'd missed) Change-Id: Ie41c0c6f32c24887b2f53ec3686c2aab73a1bfff Reviewed-on: https://go-review.googlesource.com/c/go/+/497675 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Russ Cox <rsc@golang.org>

gopherbot · 2023-05-25T21:58:22Z

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

The earlier CL 497675 for golang#60304 introduced a behavior change that, while not strictly a bug, caused a bunch of test failures in a large codebase. Rather than add behavior changes in a 10 year old package, revert to the old behavior: a context cancelation between Rows.Next reporting false and a call to Rows.Err should not result in Rows.Err returning the context error. That behavior was accidentally added in CL 497675 as part of changing how contexts and Rows iteration worked. Updates golang#60304 Updates golang#53970 Change-Id: I22f8a6a6b0b5a94b430576cf50e015efd01ec652

The earlier CL 497675 for #60304 introduced a behavior change that, while not strictly a bug, caused a bunch of test failures in a large codebase. Rather than add behavior changes in a 10 year old package, revert to the old behavior: a context cancelation between Rows.Next reporting false and a call to Rows.Err should not result in Rows.Err returning the context error. That behavior was accidentally added in CL 497675 as part of changing how contexts and Rows iteration worked. Updates #60304 Updates #53970 Change-Id: I22f8a6a6b0b5a94b430576cf50e015efd01ec652 Reviewed-on: https://go-review.googlesource.com/c/go/+/498398 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Russ Cox <rsc@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

ianlancetaylor · 2023-05-31T16:09:43Z

Is there anything else left to do for this issue?

rsc · 2023-06-07T14:05:37Z

All set.

… drivers Previously we allowed drivers to modify the row buffer used to scan values when closing Rows. This is no longer acceptable and can lead to data races. Fixes #23519 Change-Id: I91820a6266ffe52f95f40bb47307d375727715af Reviewed-on: https://go-review.googlesource.com/89936 Run-TryBot: Daniel Theophanes <kardianos@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>

rsc mentioned this issue May 19, 2023

database/sql: Using RawBytes and cancelling query context is inherently racy #53970

Closed

This was referenced May 19, 2023

database/sql: sql.RawBytes is not compatible with contexts [1.19 backport] #60307

Closed

database/sql: sql.RawBytes is not compatible with contexts [1.20 backport] #60308

Closed

rsc added this to the Go1.21 milestone May 19, 2023

rsc added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels May 19, 2023

methane mentioned this issue May 24, 2023

Revert double buffering. go-sql-driver/mysql#1435

Open

bradfitz mentioned this issue May 25, 2023

patch: database/sql: make RawBytes safely usable with contexts tailscale/go#63

Closed

rsc closed this as completed Jun 7, 2023

zikaeroh mentioned this issue Jun 22, 2023

database/sql: sqlboiler-using project fails with cancellation errors after context/RawBytes CL when many tests run #60932

Closed

fernando-az-alpaca mentioned this issue Dec 1, 2023

database/sql: possible deadlock on Rows.awaitDone()/Rows.Err()/Rows.Close() after RawBytes change #64498

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bradfitz commented May 19, 2023 •

edited

rsc commented May 19, 2023

rsc commented May 19, 2023 •

edited

rsc commented May 19, 2023

rsc commented May 19, 2023

gopherbot commented May 19, 2023

renthraysk commented May 19, 2023 •

edited

heschi commented May 22, 2023

rsc commented May 22, 2023

gopherbot commented May 23, 2023

heschi commented May 24, 2023

gopherbot commented May 25, 2023

ianlancetaylor commented May 31, 2023

rsc commented Jun 7, 2023

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

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

Comments

bradfitz commented May 19, 2023 • edited

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

Does this issue reproduce with the latest release?

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

rsc commented May 19, 2023

rsc commented May 19, 2023 • edited

rsc commented May 19, 2023

rsc commented May 19, 2023

gopherbot commented May 19, 2023

renthraysk commented May 19, 2023 • edited

heschi commented May 22, 2023

rsc commented May 22, 2023

gopherbot commented May 23, 2023

heschi commented May 24, 2023

gopherbot commented May 25, 2023

ianlancetaylor commented May 31, 2023

rsc commented Jun 7, 2023

bradfitz commented May 19, 2023 •

edited

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

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

rsc commented May 19, 2023 •

edited

renthraysk commented May 19, 2023 •

edited