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

Problem: memiavl is not protected against concurrent writing #1100

Merged
merged 10 commits into from
Jul 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
- [#1042](https://github.com/crypto-org-chain/cronos/pull/1042) call Close method on app to cleanup resource on graceful shutdown ([ethermint commit](https://github.com/crypto-org-chain/ethermint/commit/0ea7b86532a1144f229961f94b4524d5889e874d)).
- [#1083](https://github.com/crypto-org-chain/cronos/pull/1083) memiavl support both sdk 46 and 47 root hash rules.
- [#1091](https://github.com/crypto-org-chain/cronos/pull/1091) memiavl support rollback.
- [#]() memiavl support read-only mode, and grab exclusive lock for write mode.
yihuang marked this conversation as resolved.
Show resolved Hide resolved

### Improvements

Expand Down
122 changes: 101 additions & 21 deletions memiavl/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (

const DefaultSnapshotInterval = 1000

var errReadOnly = errors.New("db is read-only")

// DB implements DB-like functionalities on top of MultiTree:
// - async snapshot rewriting
// - Write-ahead-log
Expand All @@ -34,8 +36,10 @@ const DefaultSnapshotInterval = 1000
// ```
type DB struct {
MultiTree
dir string
logger log.Logger
dir string
logger log.Logger
lockFile *os.File
readOnly bool

// result channel of snapshot rewrite goroutine
snapshotRewriteChan chan snapshotResult
Expand Down Expand Up @@ -72,6 +76,7 @@ type Options struct {
Logger log.Logger
CreateIfMissing bool
InitialVersion uint32
ReadOnly bool
// the initial stores when initialize the empty instance
InitialStores []string
SnapshotKeepRecent uint32
Expand All @@ -94,12 +99,60 @@ type Options struct {
LoadForOverwriting bool
}

func (opts Options) Validate() error {
if opts.ReadOnly && opts.CreateIfMissing {
return errors.New("can't create db in read-only mode")
}

if opts.ReadOnly && opts.LoadForOverwriting {
return errors.New("can't rollback db in read-only mode")
}

return nil
}

const (
SnapshotPrefix = "snapshot-"
SnapshotDirLen = len(SnapshotPrefix) + 20
)

func Load(dir string, opts Options) (*DB, error) {
if opts.Logger == nil {
opts.Logger = log.NewNopLogger()
}

if opts.SnapshotInterval == 0 {
opts.SnapshotInterval = DefaultSnapshotInterval
}

if err := opts.Validate(); err != nil {
return nil, fmt.Errorf("invalid options: %w", err)
}

if err := tryReadMetadata(dir); err != nil {
if opts.CreateIfMissing && os.IsNotExist(err) {
err = initEmptyDB(dir, opts.InitialVersion)
}
if err != nil {
return nil, fmt.Errorf("fail to load db: %w", err)
}
}

var (
err error
lockFile *os.File
)
if !opts.ReadOnly {
// grab exclusive lock
lockFile, err = os.OpenFile(lockFilePath(dir), os.O_RDWR|os.O_CREATE, 0644)
Fixed Show fixed Hide fixed
if err != nil {
return nil, fmt.Errorf("fail to open lock file: %w", err)
}
if err := LockOrUnlock(lockFile, true); err != nil {
return nil, fmt.Errorf("fail to lock db: %w", err)
}
}

snapshot := "current"
if opts.TargetVersion > 0 {
// find the biggest snapshot version that's less than or equal to the target version
Expand All @@ -113,15 +166,7 @@ func Load(dir string, opts Options) (*DB, error) {
path := filepath.Join(dir, snapshot)
mtree, err := LoadMultiTree(path, opts.ZeroCopy, opts.CacheSize)
if err != nil {
if opts.CreateIfMissing && os.IsNotExist(err) {
if err := initEmptyDB(dir, opts.InitialVersion); err != nil {
return nil, err
}
mtree, err = LoadMultiTree(path, opts.ZeroCopy, opts.CacheSize)
}
if err != nil {
return nil, err
}
return nil, err
}

wal, err := OpenWAL(walPath(dir), &wal.Options{NoCopy: true, NoSync: true})
Expand Down Expand Up @@ -176,6 +221,8 @@ func Load(dir string, opts Options) (*DB, error) {
MultiTree: *mtree,
logger: opts.Logger,
dir: dir,
lockFile: lockFile,
readOnly: opts.ReadOnly,
wal: wal,
walChanSize: opts.AsyncCommitBuffer,
snapshotKeepRecent: opts.SnapshotKeepRecent,
Expand All @@ -184,15 +231,7 @@ func Load(dir string, opts Options) (*DB, error) {
triggerStateSyncExport: opts.TriggerStateSyncExport,
}

if db.logger == nil {
db.logger = log.NewNopLogger()
}

if db.snapshotInterval == 0 {
db.snapshotInterval = DefaultSnapshotInterval
}

if db.Version() == 0 && len(opts.InitialStores) > 0 {
if !db.readOnly && db.Version() == 0 && len(opts.InitialStores) > 0 {
// do the initial upgrade with the `opts.InitialStores`
var upgrades []*TreeNameUpgrade
for _, name := range opts.InitialStores {
Expand All @@ -206,6 +245,11 @@ func Load(dir string, opts Options) (*DB, error) {
return db, nil
}

// ReadOnly returns whether the DB is opened in read-only mode.
func (db *DB) ReadOnly() bool {
return db.readOnly
}

// SetInitialVersion wraps `MultiTree.SetInitialVersion`.
// it do an immediate snapshot rewrite, because we can't use wal log to record this change,
// because we need it to convert versions to wal index in the first place.
Expand All @@ -230,6 +274,10 @@ func (db *DB) ApplyUpgrades(upgrades []*TreeNameUpgrade) error {
db.mtx.Lock()
defer db.mtx.Unlock()

if db.readOnly {
return errReadOnly
}

if err := db.MultiTree.ApplyUpgrades(upgrades); err != nil {
return err
}
Expand Down Expand Up @@ -351,6 +399,10 @@ func (db *DB) Commit(changeSets []*NamedChangeSet) ([]byte, int64, error) {
db.mtx.Lock()
defer db.mtx.Unlock()

if db.readOnly {
return nil, 0, errReadOnly
}

if err := db.checkAsyncTasks(); err != nil {
return nil, 0, err
}
Expand Down Expand Up @@ -457,6 +509,10 @@ func (db *DB) RewriteSnapshot() error {
db.mtx.Lock()
defer db.mtx.Unlock()

if db.readOnly {
return errReadOnly
}

snapshotDir := snapshotName(db.lastCommitInfo.Version)
tmpDir := snapshotDir + "-tmp"
path := filepath.Join(db.dir, tmpDir)
Expand Down Expand Up @@ -525,6 +581,10 @@ func (db *DB) RewriteSnapshotBackground() error {
db.mtx.Lock()
defer db.mtx.Unlock()

if db.readOnly {
return errReadOnly
}

return db.rewriteSnapshotBackground()
}

Expand Down Expand Up @@ -571,7 +631,13 @@ func (db *DB) Close() error {
db.mtx.Lock()
defer db.mtx.Unlock()

return errors.Join(db.waitAsyncCommit(), db.MultiTree.Close(), db.wal.Close())
errs := []error{
db.waitAsyncCommit(), db.MultiTree.Close(), db.wal.Close(),
}
if db.lockFile != nil {
errs = append(errs, LockOrUnlock(db.lockFile, false), db.lockFile.Close())
}
return errors.Join(errs...)
}

// TreeByName wraps MultiTree.TreeByName to add a lock.
Expand Down Expand Up @@ -611,6 +677,10 @@ func (db *DB) ApplyChangeSet(changeSets []*NamedChangeSet, updateCommitInfo bool
db.mtx.Lock()
defer db.mtx.Unlock()

if db.readOnly {
return nil, 0, errReadOnly
}

return db.MultiTree.ApplyChangeSet(changeSets, updateCommitInfo)
}

Expand Down Expand Up @@ -793,6 +863,16 @@ func atomicRemoveDir(path string) error {
return os.RemoveAll(tmpPath)
}

// tryReadMetadata try to read the metadata of current snapshot to checks if the db exists
func tryReadMetadata(dir string) error {
_, err := os.ReadFile(filepath.Join(dir, "current", MetadataFileName))
yihuang marked this conversation as resolved.
Show resolved Hide resolved
return err
}

func lockFilePath(dir string) string {
return filepath.Join(dir, "LOCK")
}

type walEntry struct {
index uint64
data *WALEntry
Expand Down
1 change: 1 addition & 0 deletions memiavl/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func (db *DB) Snapshot(height uint64, protoWriter protoio.Writer) (returnErr err
db, err := Load(db.dir, Options{
TargetVersion: version,
ZeroCopy: true,
ReadOnly: true,
})
if err != nil {
return errors.Wrapf(err, "invalid height: %d", height)
Expand Down
21 changes: 21 additions & 0 deletions memiavl/lock.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package memiavl

import (
"io"
"os"
"syscall"
)

// LockOrUnlock grab or release the exclusive lock on an opened file
func LockOrUnlock(file *os.File, lock bool) error {
op := int16(syscall.F_WRLCK)
if !lock {
op = syscall.F_UNLCK
}
return syscall.FcntlFlock(file.Fd(), syscall.F_SETLK, &syscall.Flock_t{
Start: 0,
Len: 0,
Type: op,
Whence: io.SeekStart,
})
}
2 changes: 1 addition & 1 deletion store/rootmulti/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ func (rs *Store) Query(req abci.RequestQuery) abci.ResponseQuery {
db := rs.db
if version != rs.lastCommitInfo.Version {
var err error
db, err = memiavl.Load(rs.dir, memiavl.Options{TargetVersion: uint32(version)})
db, err = memiavl.Load(rs.dir, memiavl.Options{TargetVersion: uint32(version), ReadOnly: true})

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion

Potential integer overflow by integer type conversion
if err != nil {
return sdkerrors.QueryResult(err, false)
}
Expand Down