Merge branch 'cyphar-548-fix-errors-with-unsynced-metadata'

3 files changed, 98 insertions, 49 deletions

diff --git a/db.go b/db.go
index 9048f6c..1223493 100644
--- a/db.go
+++ b/db.go
@@ -205,9 +205,17 @@ func Open(path string, mode os.FileMode, options *Options) (*DB, error) {
 		if _, err := db.file.ReadAt(buf[:], 0); err == nil {
 			m := db.pageInBuffer(buf[:], 0).meta()
 			if err := m.validate(); err != nil {
-				return nil, err
+				// If we can't read the page size, we can assume it's the same
+				// as the OS -- since that's how the page size was chosen in the
+				// first place.
+				//
+				// If the first page is invalid and this OS uses a different
+				// page size than what the database was created with then we
+				// are out of luck and cannot access the database.
+				db.pageSize = os.Getpagesize()
+			} else {
+				db.pageSize = int(m.pageSize)
 			}
-			db.pageSize = int(m.pageSize)
 		}
 	}
 
@@ -274,12 +282,13 @@ func (db *DB) mmap(minsz int) error {
 	db.meta0 = db.page(0).meta()
 	db.meta1 = db.page(1).meta()
 
-	// Validate the meta pages.
-	if err := db.meta0.validate(); err != nil {
-		return err
-	}
-	if err := db.meta1.validate(); err != nil {
-		return err
+	// Validate the meta pages. We only return an error if both meta pages fail
+	// validation, since meta0 failing validation means that it wasn't saved
+	// properly -- but we can recover using meta1. And vice-versa.
+	err0 := db.meta0.validate()
+	err1 := db.meta1.validate()
+	if err0 != nil && err1 != nil {
+		return err0
 	}
 
 	return nil
@@ -351,6 +360,7 @@ func (db *DB) init() error {
 		m.root = bucket{root: 3}
 		m.pgid = 4
 		m.txid = txid(i)
+		m.checksum = m.sum64()
 	}
 
 	// Write an empty freelist at page 3.
@@ -790,10 +800,26 @@ func (db *DB) pageInBuffer(b []byte, id pgid) *page {
 
 // meta retrieves the current meta page reference.
 func (db *DB) meta() *meta {
-	if db.meta0.txid > db.meta1.txid {
-		return db.meta0
+	// We have to return the meta with the highest txid which doesn't fail
+	// validation. Otherwise, we can cause errors when in fact the database is
+	// in a consistent state. metaA is the one with the higher txid.
+	metaA := db.meta0
+	metaB := db.meta1
+	if db.meta1.txid > db.meta0.txid {
+		metaA = db.meta1
+		metaB = db.meta0
+	}
+
+	// Use higher meta page if valid. Otherwise fallback to previous, if valid.
+	if err := metaA.validate(); err == nil {
+		return metaA
+	} else if err := metaB.validate(); err == nil {
+		return metaB
 	}
-	return db.meta1
+
+	// This should never be reached, because both meta1 and meta0 were validated
+	// on mmap() and we do fsync() on every write.
+	panic("bolt.DB.meta(): invalid meta pages")
 }
 
 // allocate returns a contiguous block of memory starting at a given page.
@@ -954,12 +980,12 @@ type meta struct {
 
 // validate checks the marker bytes and version of the meta page to ensure it matches this binary.
 func (m *meta) validate() error {
-	if m.checksum != 0 && m.checksum != m.sum64() {
-		return ErrChecksum
-	} else if m.magic != magic {
+	if m.magic != magic {
 		return ErrInvalid
 	} else if m.version != version {
 		return ErrVersionMismatch
+	} else if m.checksum != 0 && m.checksum != m.sum64() {
+		return ErrChecksum
 	}
 	return nil
 }
diff --git a/db_test.go b/db_test.go
index e31b865..74ff93a 100644
--- a/db_test.go
+++ b/db_test.go
@@ -45,7 +45,7 @@ type meta struct {
 	_        uint32
 	_        [16]byte
 	_        uint64
-	_        uint64
+	pgid     uint64
 	_        uint64
 	checksum uint64
 }
@@ -85,20 +85,15 @@ func TestOpen_ErrNotExists(t *testing.T) {
 	}
 }
 
-// Ensure that opening a file with wrong checksum returns ErrChecksum.
-func TestOpen_ErrChecksum(t *testing.T) {
-	buf := make([]byte, pageSize)
-	meta := (*meta)(unsafe.Pointer(&buf[0]))
-	meta.magic = magic
-	meta.version = version
-	meta.checksum = 123
-
+// Ensure that opening a file that is not a Bolt database returns ErrInvalid.
+func TestOpen_ErrInvalid(t *testing.T) {
 	path := tempfile()
+
 	f, err := os.Create(path)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if _, err := f.WriteAt(buf, pageHeaderSize); err != nil {
+	if _, err := fmt.Fprintln(f, "this is not a bolt database"); err != nil {
 		t.Fatal(err)
 	}
 	if err := f.Close(); err != nil {
@@ -106,54 +101,81 @@ func TestOpen_ErrChecksum(t *testing.T) {
 	}
 	defer os.Remove(path)
 
-	if _, err := bolt.Open(path, 0666, nil); err != bolt.ErrChecksum {
+	if _, err := bolt.Open(path, 0666, nil); err != bolt.ErrInvalid {
 		t.Fatalf("unexpected error: %s", err)
 	}
 }
 
-// Ensure that opening a file that is not a Bolt database returns ErrInvalid.
-func TestOpen_ErrInvalid(t *testing.T) {
-	path := tempfile()
+// Ensure that opening a file with two invalid versions returns ErrVersionMismatch.
+func TestOpen_ErrVersionMismatch(t *testing.T) {
+	if pageSize != os.Getpagesize() {
+		t.Skip("page size mismatch")
+	}
 
-	f, err := os.Create(path)
-	if err != nil {
+	// Create empty database.
+	db := MustOpenDB()
+	path := db.Path()
+	defer db.MustClose()
+
+	// Close database.
+	if err := db.DB.Close(); err != nil {
 		t.Fatal(err)
 	}
-	if _, err := fmt.Fprintln(f, "this is not a bolt database"); err != nil {
+
+	// Read data file.
+	buf, err := ioutil.ReadFile(path)
+	if err != nil {
 		t.Fatal(err)
 	}
-	if err := f.Close(); err != nil {
+
+	// Rewrite meta pages.
+	meta0 := (*meta)(unsafe.Pointer(&buf[pageHeaderSize]))
+	meta0.version++
+	meta1 := (*meta)(unsafe.Pointer(&buf[pageSize+pageHeaderSize]))
+	meta1.version++
+	if err := ioutil.WriteFile(path, buf, 0666); err != nil {
 		t.Fatal(err)
 	}
-	defer os.Remove(path)
 
-	if _, err := bolt.Open(path, 0666, nil); err != bolt.ErrInvalid {
+	// Reopen data file.
+	if _, err := bolt.Open(path, 0666, nil); err != bolt.ErrVersionMismatch {
 		t.Fatalf("unexpected error: %s", err)
 	}
 }
 
-// Ensure that opening a file created with a different version of Bolt returns
-// ErrVersionMismatch.
-func TestOpen_ErrVersionMismatch(t *testing.T) {
-	buf := make([]byte, pageSize)
-	meta := (*meta)(unsafe.Pointer(&buf[0]))
-	meta.magic = magic
-	meta.version = version + 100
+// Ensure that opening a file with two invalid checksums returns ErrChecksum.
+func TestOpen_ErrChecksum(t *testing.T) {
+	if pageSize != os.Getpagesize() {
+		t.Skip("page size mismatch")
+	}
 
-	path := tempfile()
-	f, err := os.Create(path)
-	if err != nil {
+	// Create empty database.
+	db := MustOpenDB()
+	path := db.Path()
+	defer db.MustClose()
+
+	// Close database.
+	if err := db.DB.Close(); err != nil {
 		t.Fatal(err)
 	}
-	if _, err := f.WriteAt(buf, pageHeaderSize); err != nil {
+
+	// Read data file.
+	buf, err := ioutil.ReadFile(path)
+	if err != nil {
 		t.Fatal(err)
 	}
-	if err := f.Close(); err != nil {
+
+	// Rewrite meta pages.
+	meta0 := (*meta)(unsafe.Pointer(&buf[pageHeaderSize]))
+	meta0.pgid++
+	meta1 := (*meta)(unsafe.Pointer(&buf[pageSize+pageHeaderSize]))
+	meta1.pgid++
+	if err := ioutil.WriteFile(path, buf, 0666); err != nil {
 		t.Fatal(err)
 	}
-	defer os.Remove(path)
 
-	if _, err := bolt.Open(path, 0666, nil); err != bolt.ErrVersionMismatch {
+	// Reopen data file.
+	if _, err := bolt.Open(path, 0666, nil); err != bolt.ErrChecksum {
 		t.Fatalf("unexpected error: %s", err)
 	}
 }
diff --git a/errors.go b/errors.go
index 6883786..a3620a3 100644
--- a/errors.go
+++ b/errors.go
@@ -12,7 +12,8 @@ var (
 	// already open.
 	ErrDatabaseOpen = errors.New("database already open")
 
-	// ErrInvalid is returned when a data file is not a Bolt-formatted database.
+	// ErrInvalid is returned when both meta pages on a database are invalid.
+	// This typically occurs when a file is not a bolt database.
 	ErrInvalid = errors.New("invalid database")
 
 	// ErrVersionMismatch is returned when the data file was created with a