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

"SELECT 1;;" means (rc *SQLiteRows) Next() enters an infinite loop (SQLITE_MISUSE) #1161

Closed
otoolep opened this issue May 5, 2023 · 8 comments

Comments

@otoolep
Copy link
Contributor

otoolep commented May 5, 2023

Spotted by rqlite/rqlite#1250

Apply the diff below, execute time go test -run TestQueryer and it will never return. Without the diff the test it runs fine, and exits normally. From instrumenting my own code (rqlite) it seems rows.Next() never returns an error. Should the code be more robust against this, or am I doing something wrong?

$ git diff
diff --git a/sqlite3_test.go b/sqlite3_test.go
index 326361e..df317f2 100644
--- a/sqlite3_test.go
+++ b/sqlite3_test.go
@@ -1114,9 +1114,7 @@ func TestQueryer(t *testing.T) {
        if err != nil {
                t.Error("Failed to call db.Exec:", err)
        }
-       rows, err := db.Query(`
-               select id from foo order by id;
-       `)
+       rows, err := db.Query(`SELECT 1;;`)
        if err != nil {
                t.Error("Failed to call db.Query:", err)
        }
@otoolep
Copy link
Contributor Author

otoolep commented May 5, 2023

Digging into this I see rv := C._sqlite3_step_internal(rc.s.s) return (21) SQLITE_MISUSE which sends the code down the if rv != C.SQLITE_ROW branch. Then it calls rv = C.sqlite3_reset(rc.s.s) which returns (0) SQLITE_OK. And then nil is returned by Next(). Then Next() is called again, and we keep going forever.

According to the SQLite docs, getting (21) SQLITE_MISUSE is bad.

If SQLite ever returns SQLITE_MISUSE from any interface, that means that the application is incorrectly coded and needs to be fixed. Do not ship an application that sometimes returns SQLITE_MISUSE from a standard SQLite interface because that application contains potentially serious bugs.

Does db.Query("SELECT 1;;") violate the API for go-sqlite3? If so, how?

@otoolep otoolep changed the title "SELECT 1;;" means (rc *SQLiteRows) Next() enters an infinite loop "SELECT 1;;" means (rc *SQLiteRows) Next() enters an infinite loop (SQL_MISUSE) May 5, 2023
@otoolep otoolep changed the title "SELECT 1;;" means (rc *SQLiteRows) Next() enters an infinite loop (SQL_MISUSE) "SELECT 1;;" means (rc *SQLiteRows) Next() enters an infinite loop (SQLITE_MISUSE) May 5, 2023
@rittneje
Copy link
Collaborator

rittneje commented May 5, 2023

This is the same underlying issue as discussed in #950 and #1133. Namely, this library is not using sqlite3_prepare_v2 correctly.

That said, passing multiple statements to Query is not something I would recommend, as the behavior is ill-defined.

@otoolep
Copy link
Contributor Author

otoolep commented May 5, 2023

Dupe of #950

@otoolep otoolep closed this as completed May 5, 2023
@otoolep
Copy link
Contributor Author

otoolep commented May 5, 2023

OK, thanks. So is there a reason #950 isn't fixed? You seem to have identified the issue, but it's not clear if it's much more difficult to fix that it seems.

@otoolep
Copy link
Contributor Author

otoolep commented May 5, 2023

I can patch my copy of my fork, but it's just rather unfortunate that a simple string can bring down such an important library as this one.

@rittneje
Copy link
Collaborator

rittneje commented May 5, 2023

Because I have not had the time to deal with it. Feel free to raise a PR and I'll take a look.

@otoolep
Copy link
Contributor Author

otoolep commented May 5, 2023

OK, let me see what I can do.

@danlzh
Copy link

danlzh commented Jan 10, 2024

Currently is there a way to detect this? Encountered the same issue when executing malformed empty SQL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants