Skip to content

Commit

Permalink
Fix runtime panic for opening lockfile if parent dir got removed
Browse files Browse the repository at this point in the history
If the lockfile directory gets removed but the container runtime is
still running with an open storage instance, then we can trigger a
runtime panic by:

1. Starting CRI-O
2. Removing the lockfile dir, e.g.:
   `sudo rm -rf /var/lib/containers/storage/overlay-images`
3. Running `crictl images`

```
panic: error opening "/var/lib/containers/storage/overlay-images/images.lock": no such file or directory

goroutine 93 [running]:
github.com/containers/storage/pkg/lockfile.(*lockfile).lock(0xc0006b83c0, 0x2000000)
        /go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go:107 +0x388
github.com/containers/storage/pkg/lockfile.(*lockfile).RLock(0xc0006b83c0)
        /go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go:146 +0x37
github.com/containers/storage.(*imageStore).RLock(0xc00006e420)
        /go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/images.go:778 +0x33
github.com/containers/storage.(*store).Images(0xc0003f3b80, 0x0, 0x0, 0x0, 0x0, 0x0)
        /go/src/github.com/cri-o/cri-o/vendor/github.com/containers/storage/store.go:3089 +0x1dc
```

We now ensure the parent lockfile directory, but only if it does not
exist to not degrade the performance of c/stroage.

Unit tests around that case have been added as well.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
  • Loading branch information
saschagrunert committed Jul 7, 2021
1 parent a52896d commit 16de6d3
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 3 deletions.
26 changes: 23 additions & 3 deletions pkg/lockfile/lockfile_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package lockfile
import (
"fmt"
"os"
"path/filepath"
"sync"
"time"

Expand Down Expand Up @@ -33,11 +34,30 @@ type lockfile struct {
// descriptor. Note that the path is opened read-only when ro is set. If ro
// is unset, openLock will open the path read-write and create the file if
// necessary.
func openLock(path string, ro bool) (int, error) {
func openLock(path string, ro bool) (fd int, err error) {
if ro {
return unix.Open(path, os.O_RDONLY|unix.O_CLOEXEC, 0)
fd, err = unix.Open(path, os.O_RDONLY|unix.O_CLOEXEC, 0)
} else {
fd, err = unix.Open(path,
os.O_RDWR|unix.O_CLOEXEC|os.O_CREATE,
unix.S_IRUSR|unix.S_IWUSR|unix.S_IRGRP|unix.S_IROTH,
)
}

if err == nil {
return
}

// the directory of the lockfile seems to be removed, try to create it
if os.IsNotExist(err) {
if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil {
return fd, errors.Wrap(err, "creating locker directory")
}

return openLock(path, ro)
}
return unix.Open(path, os.O_RDWR|unix.O_CLOEXEC|os.O_CREATE, unix.S_IRUSR|unix.S_IWUSR|unix.S_IRGRP|unix.S_IROTH)

return
}

// createLockerForPath returns a Locker object, possibly (depending on the platform)
Expand Down
65 changes: 65 additions & 0 deletions pkg/lockfile/lockfile_unix_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// +build linux solaris darwin freebsd

package lockfile

import (
"io/ioutil"
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/require"
)

func TestOpenLock(t *testing.T) {
t.Parallel()

for _, tc := range []struct {
name string
prepare func() (path string, readOnly bool)
}{
{
name: "file exists (read/write)",
prepare: func() (string, bool) {
tempFile, err := ioutil.TempFile("", "lock-")
require.NoError(t, err)
return tempFile.Name(), false
},
},
{
name: "file exists readonly (readonly)",
prepare: func() (string, bool) {
tempFile, err := ioutil.TempFile("", "lock-")
require.NoError(t, err)
return tempFile.Name(), true
},
},
{
name: "base dir exists (read/write)",
prepare: func() (string, bool) {
tempDir := os.TempDir()
require.DirExists(t, tempDir)
return filepath.Join(tempDir, "test-1.lock"), false
},
},
{
name: "base dir not exists (read/write)",
prepare: func() (string, bool) {
tempDir, err := ioutil.TempDir("", "lock-")
require.NoError(t, err)
return filepath.Join(tempDir, "subdir", "test-1.lock"), false
},
},
} {
path, readOnly := tc.prepare()

_, err := openLock(path, readOnly)

require.NoError(t, err, tc.name)

_, err = openLock(path, readOnly)
require.NoError(t, err)

require.Nil(t, os.RemoveAll(path))
}
}

0 comments on commit 16de6d3

Please sign in to comment.