Data race when accessing dirties map in hashdb.Database.commit

[magicxyyz]

System information

Commit hash: c8a2202 (current master)

Issue description

In trie/triedb/hashdb.Database.commit a key is read from dirites map without locking db lock, while trie/tiredb/hashdb.Database.dereference may modify the map concurrently resulting in concurrent map read and map write panic.

Possible scenario affected by the issue

One scenario affected by the race is when a new block state is committed and concurrently an rpc is called (e.g. debug_traceBlockByNumber) which executes eth.StateAtBlock querying a state that is available in live database and then executes tracers.StateReleaseFunc which dereferences a node.

Code pointers

• Read access to dirties map:
  

  go-ethereum/trie/triedb/hashdb/database.go

  Lines 479 to 484 in c8a2202

  	func (db *Database) commit(hash common.Hash, batch ethdb.Batch, uncacher *cleaner) error {
  	// If the node does not exist, it's a previously committed node
  	node, ok := db.dirties[hash]
  	if !ok {
  	return nil
  	}

• Write access to dirties map:
  

  go-ethereum/trie/triedb/hashdb/database.go

  Line 337 in c8a2202

  delete(db.dirties, hash)

• Example of place where dereference is called in rpc handling goroutine:

go-ethereum/eth/tracers/api.go

Lines 569 to 586 in c8a2202

	func (api *API) traceBlock(ctx context.Context, block *types.Block, config *TraceConfig) ([]*txTraceResult, error) {
	if block.NumberU64() == 0 {
	return nil, errors.New("genesis is not traceable")
	}
	// Prepare base state
	parent, err := api.blockByNumberAndHash(ctx, rpc.BlockNumber(block.NumberU64()-1), block.ParentHash())
	if err != nil {
	return nil, err
	}
	reexec := defaultTraceReexec
	if config != nil && config.Reexec != nil {
	reexec = *config.Reexec
	}
	statedb, release, err := api.backend.StateAtBlock(ctx, parent, reexec, nil, true, false)
	if err != nil {
	return nil, err
	}
	defer release()

go-ethereum/eth/api_backend.go

Lines 411 to 412 in c8a2202

	func (b *EthAPIBackend) StateAtBlock(ctx context.Context, block *types.Block, reexec uint64, base *state.StateDB, readOnly bool, preferDisk bool) (*state.StateDB, tracers.StateReleaseFunc, error) {
	return b.eth.stateAtBlock(ctx, block, reexec, base, readOnly, preferDisk)

go-ethereum/eth/state_accessor.go

Lines 211 to 214 in c8a2202

	func (eth *Ethereum) stateAtBlock(ctx context.Context, block *types.Block, reexec uint64, base *state.StateDB, readOnly bool, preferDisk bool) (statedb *state.StateDB, release tracers.StateReleaseFunc, err error) {
	if eth.blockchain.TrieDB().Scheme() == rawdb.HashScheme {
	return eth.hashState(ctx, block, reexec, base, readOnly, preferDisk)
	}

go-ethereum/eth/state_accessor.go

Lines 50 to 60 in c8a2202

	if readOnly {
	// The state is available in live database, create a reference
	// on top to prevent garbage collection and return a release
	// function to deref it.
	if statedb, err = eth.blockchain.StateAt(block.Root()); err == nil {
	eth.blockchain.TrieDB().Reference(block.Root(), common.Hash{})
	return statedb, func() {
	eth.blockchain.TrieDB().Dereference(block.Root())
	}, nil
	}
	}

• Example of place where commit is called in another goroutine:
  

  go-ethereum/core/blockchain.go

  Line 1457 in c8a2202

  bc.triedb.Commit(header.Root, true)

Proposed solution

Lock db lock when accessing dirties map in hashdb.Database.commit.

[rjl493456442]

Good catch. From the function description of Commit, it said

// Note, this method is a non-synchronized mutator. It is unsafe to call this
// concurrently with other mutators.

Apparently this guarantee is not held in the case of tracer. Gonna think about how to fix it properly.