Can't prevent nil reference to Tx.meta, Tx.db in Tx.Size

[gyuho]

Hello,

Currently there's no way to prevent *bolt.Tx.Size() from referring to nil Tx.meta or Tx.db since meta and db fields are unexported. etcd calls this method only for internal consistency checking, so I doubt anyone would face this issue in production. But once it happens, it panics the whole node, and there's nothing that we can do, as an external project importing boltdb.

Here's how invalid memory address happens in etcd

We have Hash method that commits *bolt.Tx, which calls https://github.com/boltdb/bolt/blob/master/tx.go#L228

func (tx *Tx) Commit() error {

    // Finalize the transaction.
    tx.close()

And *bolt.Tx calls tx.close(), which clears these references

func (tx *Tx) close() {

    // Clear all references.
    tx.db = nil
    tx.meta = nil
    tx.root = Bucket{tx: tx}
    tx.pages = nil

Following *bolt.Tx.Size() calls would panic because tx.meta == nil && tx.db == nil

func (tx *Tx) Size() int64 {
    // tx.meta == nil, tx.db == nil
    return int64(tx.meta.pgid) * int64(tx.db.pageSize)
}

panic: runtime error: invalid memory address or nil pointer dereference

Way to reproduce:

package main

import (
    "fmt"
    "os"
    "time"

    "github.com/boltdb/bolt"
)

func main() {
    db, err := bolt.Open("db", 0600, &bolt.Options{Timeout: 1 * time.Second})
    if err != nil {
        panic(err)
    }
    defer os.Remove("db")

    var tx *bolt.Tx
    tx, err = db.Begin(true)
    if err != nil {
        panic(err)
    }
    _, err = tx.CreateBucketIfNotExists([]byte("key"))
    if err != nil {
        panic(err)
    }
    if err = tx.Commit(); err != nil {
        panic(err)
    }

    tx, err = db.Begin(true)
    if err != nil {
        panic(err)
    }
    bucket := tx.Bucket([]byte("key"))
    if err = bucket.Put([]byte("foo"), []byte("val")); err != nil {
        panic(err)
    }
    if err = tx.Commit(); err != nil {
        panic(err)
    }

    fmt.Println(tx.Size())

    // this would not panic
    // tx, err = db.Begin(true)
    // if err != nil {
    //  panic(err)
    // }
    // tx.Size())
}

/*
go run bolt.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x401339]

goroutine 1 [running]:
panic(0x4be7a0, 0xc42000a140)
    /usr/local/go/src/runtime/panic.go:500 +0x1a1
main.main()
    bolt.go:47 +0x339
exit status 2
*/

Am I missing anything? Or could we add a method just to tell tx.meta == nil || tx.db == nil, so that external projects can skip Size call?

Thanks a lot!

[chrsm]

Is there a specific reason the size of the transaction cannot simply be determined prior to committing it?

[gyuho]

@chrsm We try not to commit too often batching our commits, if I understand your comment correctly.

On second thought, we might just call tx, err = db.Begin(true) before calling tx.Size, so that we just update the tx.meta and tx.db with new reference.

[gyuho]

Closing via etcd-io/etcd#6662.

Think we can prevent in application layer. Thanks!

[gyuho]

Btw *bolt.Tx.DB() != nil was what I needed. No need to add another method. Thanks!