From cbbc2621612efd8fed43de40e220dad98d974702 Mon Sep 17 00:00:00 2001
From: Audrius Butkevicius <audrius.butkevicius@gmail.com>
Date: Wed, 19 Aug 2020 12:13:44 +0100
Subject: [PATCH] lib/db: Dont recurse on flush (fixes #6905) (#6906)

Or else we crash.

Co-authored-by: Jakob Borg <jakob@kastelo.net>
Co-authored-by: Simon Frei <freisim93@gmail.com>
---
 lib/db/backend/leveldb_backend.go | 10 ++++++-
 lib/db/db_test.go                 | 48 +++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/lib/db/backend/leveldb_backend.go b/lib/db/backend/leveldb_backend.go
index 493f217cc..a4e9e1f21 100644
--- a/lib/db/backend/leveldb_backend.go
+++ b/lib/db/backend/leveldb_backend.go
@@ -75,6 +75,7 @@ func (b *leveldbBackend) NewWriteTransaction(hooks ...CommitHook) (WriteTransact
 		batch:           new(leveldb.Batch),
 		rel:             rel,
 		commitHooks:     hooks,
+		inFlush:         false,
 	}, nil
 }
 
@@ -147,6 +148,7 @@ type leveldbTransaction struct {
 	batch       *leveldb.Batch
 	rel         *releaser
 	commitHooks []CommitHook
+	inFlush     bool
 }
 
 func (t *leveldbTransaction) Delete(key []byte) error {
@@ -177,13 +179,19 @@ func (t *leveldbTransaction) Release() {
 
 // checkFlush flushes and resets the batch if its size exceeds the given size.
 func (t *leveldbTransaction) checkFlush(size int) error {
-	if len(t.batch.Dump()) < size {
+	// Hooks might put values in the database, which triggers a checkFlush which might trigger a flush,
+	// which might trigger the hooks.
+	// Don't recurse...
+	if t.inFlush || len(t.batch.Dump()) < size {
 		return nil
 	}
 	return t.flush()
 }
 
 func (t *leveldbTransaction) flush() error {
+	t.inFlush = true
+	defer func() { t.inFlush = false }()
+
 	for _, hook := range t.commitHooks {
 		if err := hook(t); err != nil {
 			return err
diff --git a/lib/db/db_test.go b/lib/db/db_test.go
index 7b6ab0812..3aceb4eaf 100644
--- a/lib/db/db_test.go
+++ b/lib/db/db_test.go
@@ -9,6 +9,8 @@ package db
 import (
 	"bytes"
 	"context"
+	"fmt"
+	"os"
 	"testing"
 
 	"github.com/syncthing/syncthing/lib/db/backend"
@@ -795,6 +797,52 @@ func TestUpdateTo14(t *testing.T) {
 	}
 }
 
+func TestFlushRecursion(t *testing.T) {
+	// Verify that a commit hook can write to the transaction without
+	// causing another flush and thus recursion.
+
+	// Badger doesn't work like this.
+	if os.Getenv("USE_BADGER") != "" {
+		t.Skip("Not supported on Badger")
+	}
+
+	db := NewLowlevel(backend.OpenMemory())
+	defer db.Close()
+
+	// A commit hook that writes a small piece of data to the transaction.
+	hookFired := 0
+	hook := func(tx backend.WriteTransaction) error {
+		err := tx.Put([]byte(fmt.Sprintf("hook-key-%d", hookFired)), []byte(fmt.Sprintf("hook-value-%d", hookFired)))
+		if err != nil {
+			t.Fatal(err)
+		}
+		hookFired++
+		return nil
+	}
+
+	// A transaction.
+	tx, err := db.NewWriteTransaction(hook)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer tx.Release()
+
+	// Write stuff until the transaction flushes, thus firing the hook.
+	i := 0
+	for hookFired == 0 {
+		err := tx.Put([]byte(fmt.Sprintf("key-%d", i)), []byte(fmt.Sprintf("value-%d", i)))
+		if err != nil {
+			t.Fatal(err)
+		}
+		i++
+	}
+
+	// The hook should have fired precisely once.
+	if hookFired != 1 {
+		t.Error("expect one hook fire, not", hookFired)
+	}
+}
+
 func numBlockLists(db *Lowlevel) (int, error) {
 	it, err := db.Backend.NewPrefixIterator([]byte{KeyTypeBlockList})
 	if err != nil {