p4u1/docker-cli
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

layer: protect from same-name races

Browse Source 
As pointed out by Tonis, there's a race between ReleaseRWLayer()
and GetRWLayer():

```
----- goroutine 1 -----               ----- goroutine 2 -----
ReleaseRWLayer()
  m := ls.mounts[l.Name()]
  ...
  m.deleteReference(l)
  m.hasReferences()
  ...                                 GetRWLayer()
  ...                                   mount := ls.mounts[id]
  ls.driver.Remove(m.mountID)
  ls.store.RemoveMount(m.name)          return mount.getReference()
  delete(ls.mounts, m.Name())
-----------------------               -----------------------
```

When something like this happens, GetRWLayer will return
an RWLayer without a storage. Oops.

There might be more races like this, and it seems the best
solution is to lock by layer id/name by using pkg/locker.

With this in place, name collision could not happen, so remove
the part of previous commit that protected against it in
CreateRWLayer (temporary nil assigmment and associated rollback).

So, now we have
* layerStore.mountL sync.Mutex to protect layerStore.mount map[]
  (against concurrent access);
* mountedLayer's embedded `sync.Mutex` to protect its references map[];
* layerStore.layerL (which I haven't touched);
* per-id locker, to avoid name conflicts and concurrent operations
  on the same rw layer.

The whole rig seems to look more readable now (mutexes use is
straightforward, no nested locks).

Reported-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit af433dd200f8287305b1531d5058780be36b7e2e)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 1ebe324c6a481de86a5b84494057a639773624f1
Component: engine
This commit is contained in:
Kir Kolyshkin
2019-05-20 14:46:54 -07:00
committed by Sebastiaan van Stijn
parent 8b3a1bef0d
commit e0456faac2
1 changed files with 25 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
components/engine/layer/layer_store.go
View File

@ -10,6 +10,7 @@ import (
"github.com/docker/distribution"
"github.com/docker/docker/daemon/graphdriver"
"github.com/docker/docker/pkg/idtools"
"github.com/docker/docker/pkg/locker"
"github.com/docker/docker/pkg/plugingetter"
"github.com/docker/docker/pkg/stringid"
"github.com/docker/docker/pkg/system"
@ -36,7 +37,11 @@ type layerStore struct {
mounts map[string]*mountedLayer
mountL sync.Mutex
os string
// protect *RWLayer() methods from operating on the same name/id
locker *locker.Locker
os string
}
// StoreOptions are the options used to create a new Store instance
@ -92,6 +97,7 @@ func newStoreFromGraphDriver(root string, driver graphdriver.Driver, os string)
driver: driver,
layerMap: map[ChainID]*roLayer{},
mounts: map[string]*mountedLayer{},
locker: locker.New(),
useTarSplit: !caps.ReproducesExactDiffs,
os: os,
}
@ -191,7 +197,7 @@ func (ls *layerStore) loadLayer(layer ChainID) (*roLayer, error) {
func (ls *layerStore) loadMount(mount string) error {
ls.mountL.Lock()
defer ls.mountL.Unlock()
if m := ls.mounts[mount]; m != nil {
if _, ok := ls.mounts[mount]; ok {
return nil
}
@ -492,21 +498,15 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL
initFunc = opts.InitFunc
}
ls.locker.Lock(name)
defer ls.locker.Unlock(name)
ls.mountL.Lock()
if _, ok := ls.mounts[name]; ok {
ls.mountL.Unlock()
_, ok := ls.mounts[name]
ls.mountL.Unlock()
if ok {
return nil, ErrMountNameConflict
}
// Avoid name collision by temporary assigning nil
ls.mounts[name] = nil
ls.mountL.Unlock()
defer func() {
if err != nil {
ls.mountL.Lock()
delete(ls.mounts, name)
ls.mountL.Unlock()
}
}()
var pid string
var p *roLayer
@ -558,6 +558,9 @@ func (ls *layerStore) CreateRWLayer(name string, parent ChainID, opts *CreateRWL
}
func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) {
ls.locker.Lock(id)
defer ls.locker.Unlock(id)
ls.mountL.Lock()
mount := ls.mounts[id]
ls.mountL.Unlock()
@ -570,8 +573,9 @@ func (ls *layerStore) GetRWLayer(id string) (RWLayer, error) {
func (ls *layerStore) GetMountID(id string) (string, error) {
ls.mountL.Lock()
defer ls.mountL.Unlock()
mount := ls.mounts[id]
ls.mountL.Unlock()
if mount == nil {
return "", ErrMountDoesNotExist
}
@ -581,8 +585,12 @@ func (ls *layerStore) GetMountID(id string) (string, error) {
}
func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) {
name := l.Name()
ls.locker.Lock(name)
defer ls.locker.Unlock(name)
ls.mountL.Lock()
m := ls.mounts[l.Name()]
m := ls.mounts[name]
ls.mountL.Unlock()
if m == nil {
return []Metadata{}, nil
@ -617,7 +625,7 @@ func (ls *layerStore) ReleaseRWLayer(l RWLayer) ([]Metadata, error) {
}
ls.mountL.Lock()
delete(ls.mounts, m.Name())
delete(ls.mounts, name)
ls.mountL.Unlock()
ls.layerL.Lock()