[Question] concurrent race when LoadPolicy is called

Open
#1,116 7 comments 0 reactions 1 assignee View on GitHub

@hsluoyz is already working on this.

Since Oct 11, 2022.

Assessment

This issue has not been assessed yet.

Description

enhancement

Want to prioritize this issue? Try:

issuehunt-to-marktext


What's your scenario? What do you want to achieve?

Sometimes when a watcher calls LoadPolicy to synchronize policies, panic occurs:

fatal error: concurrent map iteration and map write
goroutine 152 [running]:
runtime.throw({0x2557e5c?, 0x3711778?})
/usr/local/Cellar/go/1.18.3/libexec/src/runtime/panic.go:992 +0x71 fp=0xc000c9b818 sp=0xc000c9b7e8 pc=0x103e7d1
runtime.mapiternext(0xc000307dd0?)
/usr/local/Cellar/go/1.18.3/libexec/src/runtime/map.go:871 +0x60a fp=0xc000c9b8a8 sp=0xc000c9b818 pc=0x1012eea
runtime.mapiterinit(0x234dae0?, 0xc00064d110, 0xc000c9ba78)
/usr/local/Cellar/go/1.18.3/libexec/src/runtime/map.go:861 +0x18c fp=0xc000c9b8c8 sp=0xc000c9b8a8 pc=0x101288c
github.com/casbin/casbin/v2/model.(*Assertion).copy(0xc0008fe880)
/Users/sasakiyori/go/pkg/mod/github.com/casbin/casbin/v2@v2.47.2/model/assertion.go:108 +0x2cf fp=0xc000c9bae8 sp=0xc000c9b8c8 pc=0x199c00f
github.com/casbin/casbin/v2/model.Model.Copy(0xc0006dd800)
/Users/sasakiyori/go/pkg/mod/github.com/casbin/casbin/v2@v2.47.2/model/model.go:384 +0x1b0 fp=0xc000c9bc58 sp=0xc000c9bae8 pc=0x19a0cd0
github.com/casbin/casbin/v2.(*Enforcer).LoadPolicy(0xc000684000)
/Users/sasakiyori/go/pkg/mod/github.com/casbin/casbin/v2@v2.47.2/enforcer.go:291 +0x5a fp=0xc000c9bdc8 sp=0xc000c9bc58 pc=0x19b76ba
github.com/casbin/casbin/v2.(*Enforcer).SetWatcher.func1({0x1c29, 0xa})
/Users/sasakiyori/go/pkg/mod/github.com/casbin/casbin/v2@v2.47.2/enforcer.go:260 +0x25 fp=0xc000c9bde8 sp=0xc000c9bdc8 pc=0x19b7645
github.com/casbin/etcd-watcher/v2.(*Watcher).startWatch(0xc000349c00)
/Users/sasakiyori/go/pkg/mod/github.com/casbin/etcd-watcher/v2@v2.1.1/watcher.go:127 +0x268 fp=0xc000c9bfc0 sp=0xc000c9bde8 pc=0x2289c28
github.com/casbin/etcd-watcher/v2.NewWatcher.func1()
/Users/sasakiyori/go/pkg/mod/github.com/casbin/etcd-watcher/v2@v2.1.1/watcher.go:67 +0x25 fp=0xc000c9bfe0 sp=0xc000c9bfc0 pc=0x22892e5
runtime.goexit()
/usr/local/Cellar/go/1.18.3/libexec/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc000c9bfe8 sp=0xc000c9bfe0 pc=0x1072e01
created by github.com/casbin/etcd-watcher/v2.NewWatcher
/Users/sasakiyori/go/pkg/mod/github.com/casbin/etcd-watcher/v2@v2.1.1/watcher.go:66 +0x2a8


I can't reproduce it every time, but I think the problem may arised here:

As the call chain shown below, when a goroutine do iteration in ast.PolicyMap from ast.copy(), any other goroutines write into this map(such as AddPolicy) may cause panic.

func (e *Enforcer) LoadPolicy() error {
	needToRebuild := false
        // panic occurs
	newModel := e.model.Copy()
	newModel.ClearPolicy()
        // ...
}

func (model Model) Copy() Model {
	newModel := NewModel()

	for sec, m := range model {
		newAstMap := make(AssertionMap)
                 // ast is a pointer of AssertionMap
		for ptype, ast := range m {
			newAstMap[ptype] = ast.copy()
		}
		newModel[sec] = newAstMap
	}

	newModel.SetLogger(model.GetLogger())
	return newModel
}

func (ast *Assertion) copy() *Assertion {
	tokens := append([]string(nil), ast.Tokens...)
	policy := make([][]string, len(ast.Policy))

	for i, p := range ast.Policy {
		policy[i] = append(policy[i], p...)
	}
	policyMap := make(map[string]int)
        // map is unsafe between goroutines
        // when it is iterating, other goroutine's write operation may cause panic
	for k, v := range ast.PolicyMap {
		policyMap[k] = v
	}

	newAst := &Assertion{
		Key:           ast.Key,
		Value:         ast.Value,
		PolicyMap:     policyMap,
		Tokens:        tokens,
		Policy:        policy,
		FieldIndexMap: ast.FieldIndexMap,
	}

	return newAst
}

And What confused me is:
After copy a new model, it will clear policy map, so the copy of PolicyMap from ast.copy() is useless.
Could you create a new copy function to eliminate these unsafe operations?

func (e *Enforcer) LoadPolicy() error {
	needToRebuild := false
	newModel := e.model.Copy()
	newModel.ClearPolicy()
        // ...
}

func (model Model) ClearPolicy() {
	for _, ast := range model["p"] {
		ast.Policy = nil
		ast.PolicyMap = map[string]int{}
	}

	for _, ast := range model["g"] {
		ast.Policy = nil
		ast.PolicyMap = map[string]int{}
	}
}
Dominant language
Go
Stars
20.4k
Forks
1.8k
Avg merge
5d 11h
Merged PRs (30d)
2

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from apache/casbin

All issues in apache/casbin

Similar issues

More Go issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.