Empty query causes loop

[joriszwart]

By mistake stupid me executed an empty query. This resulted in an (infinite?) loop and hanged my application.

The minimal example below reproduces this. This example uses an in-memory database, but it occurs with a database file as well.

Version: v1.14.7

db, err := sql.Open("sqlite3", ":memory:")
if err != nil {
	panic(err)
}
defer db.Close()

stmt, err := db.Prepare("") // empty query
if err != nil {
	panic(err)
}
defer stmt.Close()

rows, err := stmt.Query()
if err != nil {
	panic(err)
}
defer rows.Close()

count := 0
for rows.Next() {
	var dummy string
	rows.Scan(&dummy) // this can be omitted

	log.Printf("%d: [%+v]\n", count, dummy)
	count++
}

Results in:

0: []
1: []
2: []
3: []
4: []
...

I don't know what the expected behavior should be. Maybe return an error?

[rittneje]

The source of the bug is that sqlite3_prepare_v2 produces a null *sqlite3_stmt with no error when you do this. But this library does not check for that.

go-sqlite3/sqlite3.go

Lines 1749 to 1752 in d33341f

	rv := C._sqlite3_prepare_v2_internal(c.db, pquery, C.int(-1), &s, &tail)
	if rv != C.SQLITE_OK {
	return nil, c.lastError()
	}

Compounding the issue, each call to sqlite3_step will fail with SQLITE_MISUSE, but it gets dropped because of this:

go-sqlite3/sqlite3.go

Lines 2105 to 2111 in d33341f

	if rv != C.SQLITE_ROW {
	rv = C.sqlite3_reset(rc.s.s)
	if rv != C.SQLITE_OK {
	return rc.s.c.lastError()
	}
	return nil
	}

Basically, sqlite3_reset always vacuously succeeds when given a null statement, so we end up returning null from each call to Next(). Thus the loop never terminates.

In short, there are two bugs in play:

1. If sqlite3_prepare_v2 yields SQLITE_OK with a null prepared statement it should be treated as an error.
2. If sqlite3_reset yields SQLITE_OK after sqlite3_step fails we should still return an error. (Note: We can't just call lastError() in a situation like this since it might not be set properly. Maybe just convert to an ErrNo?)

[dolmen]

3900dc3 has since been applied.

[joriszwart]

I can still reproduce this using v1.14.17 (latest tag). #963 was about panic'ing when specifying an empty string.

[ganigeorgiev]

@rittneje Shouldn't simply returning io.EOF instead of nil in the no rows nextSyncLocked check resolve the issue?

Edit: I've opened #1285 with the above suggestion and the tests seem to pass.

[charlievieth]

Considering that there a few bugs in this library that are due to sqlite3_prepare_v2 not being used properly (#1161 comes to mind) and that there might be existing user programs that rely on said broken behavior has there been any discussion of creating a v2 branch that resolves these issues without maintaining backwards compatibility for programs that rely on the current (and sometimes broken) behavior?