Is this a bug in the database/sql layer? Panic on Exec of empty string

[otoolep]

Passing an empty to a string causes a panic in the database/sql layer. Is there some initialization I'm missing? I see nothing in the docs that state this should panic.

~/repos/go-empty-sql-statement/src/github/com/otoolep/go-empty-sql-statement (master)$ cat main.go         

package main                                                                                                                                         
                                                                                                                                                     
import (                                                                                                                                             
        "database/sql"                                                                                                                               
        "fmt"                                                                                                                                        
        "log"                                                                                                                                        
        "os"                                                                                                                                         
                                                                                                                                                     
        _ "github.com/mattn/go-sqlite3"                                                                                                              
)                                                                                                                                                    
                                                                                                                                                     
func main() {                                                                                                                                        
        os.Remove("foo.bar")                                                                                                                         
                                                                                                                                                     
        db, err := sql.Open("sqlite3", "foo.bar")                                                                                                    
        if err != nil {                                                                                                                              
                log.Fatalf("failed to open database: %s", err.Error())                                                                               
        }                                                                                                                                            
                                                                                                                                                     
        results, err := db.Exec("")
        if err != nil {
                log.Fatalf("failed to get results: %s", err.Error())
        }
        li, err := results.LastInsertId()
        fmt.Println(li, err)
}

~/repos/go-empty-sql-statement/src/github/com/otoolep/go-empty-sql-statement (master)$ go run main.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x4fa135]

goroutine 1 [running]:
database/sql.driverResult.LastInsertId(0x6513a0, 0xc000144000, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/philip/.gvm/gos/go1.15/src/database/sql/sql.go:3266 +0x75
main.main()
        /home/philip/repos/go-empty-sql-statement/src/github/com/otoolep/go-empty-sql-statement/main.go:24 +0xfd
exit status 2
~/repos/go-empty-sql-statement/src/github/com/otoolep/go-empty-sql-statement (master)$ 

[otoolep]

It panics when attempting to get the last insert ID, because a private variable in the SQL layer is still nil.

[rittneje]

This is a similar issue to #950. This library currently makes the (unsafe) assumption that the loop in *SQLiteConn.exec will run at least one query. However, in this case, that is not true, so we end up returning nil for driver.Result here:

go-sqlite3/sqlite3.go

Lines 830 to 832 in d33341f

	if tail == "" {
	return res, nil
	}

Note that this is possible for any query that does nothing, not just the empty string. For example, you'd get similar results with "-- just some comment".

Clearly it should not panic. However, the question is, what exactly should this do instead? Should it return an error? Should it return some vacuous driver.Result? @otoolep What would your expectations be?

[otoolep]

Thanks for the info.

Looking at the function signature -- and trying to keep breaking changes to a minimum -- I would like a driver.Result where both LastInsertId() and RowsAffected() both return 0 i.e.

&SQLiteResult{
    id: 0,
    changes:0,
}

It seems the most reasonable option to me. I hacked up my local copy of your driver, and my system no longer panics when returning such a result.

For Query, I suggest an non-nil object, where Columns() returns an empty slice ([]) or is itself nil. This could be a breaking change for some folks however.

Alternatively some sort of function, a bit like Readonly which would allow me to check a statement before I pass it to the sql/database layer, but that could be a perf hit for big statements? I wouldn't like this, but might consider it.

More info:

I hit this issue quite a while ago, but coded around it somewhat. Moving to the sql/database layer made me hit again. It's a concern to me because rqlite doesn't control what's sent over the HTTP API -- someone could send in an empty request, and the system would crash.

I cope with one case by simply checking for empty queries. But you tell me any query that does nothing could cause this -- so the space of possible queries is very large! I can't check them all. I'm surprised no-one hit this yet.

For rqlite I would be fine with the system doing nothing. In other just returning an empty response to the user -- perhaps HTTP 204 to be pedanctic. What I don't want is the panic! Basically a query that does nothing crashes rqlite. :-(

[otoolep]

My hack:

                if tail == "" {
                        if res == nil {
                                res = &SQLiteResult{
                                        id: 0,
                                        changes:0,
                                }
                        }
                        return res, nil
                }

[rittneje]

I'm thinking it might be best to have both Query and Exec simply return a special error in this case, which could then be checked via errors.Is. That way, clients still have the option of treating it as some vacuous success if they so choose.

[otoolep]

Error works for me too -- I just don't want the code to panic. No one really wants to run meaningless queries, but a crashing system makes it difficult for them to understand what has happened. An error makes it much clearer.

[otoolep]

I'm going to patch my own fork for now, until upstream fixes..