diff --git a/go.mod b/go.mod index 931c5fa904ea..c295ec65b95b 100644 --- a/go.mod +++ b/go.mod @@ -99,7 +99,7 @@ require ( github.com/imdario/mergo v0.3.13 // indirect github.com/inconshreveable/mousetrap v1.0.1 // indirect github.com/jinzhu/copier v0.3.5 // indirect - github.com/klauspost/compress v1.15.13 // indirect + github.com/klauspost/compress v1.15.14 // indirect github.com/klauspost/pgzip v1.2.6-0.20220930104621-17e8dac29df8 // indirect github.com/kr/fs v0.1.0 // indirect github.com/letsencrypt/boulder v0.0.0-20221109233200-85aa52084eaf // indirect @@ -144,3 +144,5 @@ require ( ) replace github.com/opencontainers/runc => github.com/opencontainers/runc v1.1.1-0.20220617142545-8b9452f75cbc + +replace github.com/containers/storage => github.com/mtrmac/storage v0.0.0-20230103232939-ffeaed8fa4f7 diff --git a/go.sum b/go.sum index 17bc8fe9147b..d05ec7f469cf 100644 --- a/go.sum +++ b/go.sum @@ -54,8 +54,6 @@ github.com/Azure/go-autorest/autorest/mocks v0.4.1/go.mod h1:LTp+uSrOhSkaKrUy935 github.com/Azure/go-autorest/logger v0.2.0/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZmbF5NWuPV8+WeEW8= github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= -github.com/BurntSushi/toml v0.4.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= -github.com/BurntSushi/toml v1.2.0/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/BurntSushi/toml v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak= github.com/BurntSushi/toml v1.2.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= @@ -67,7 +65,6 @@ github.com/Microsoft/go-winio v0.4.16/go.mod h1:XB6nPKklQyQ7GC9LdcBEcBl8PF76WugX github.com/Microsoft/go-winio v0.4.17-0.20210211115548-6eac466e5fa3/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84= github.com/Microsoft/go-winio v0.4.17-0.20210324224401-5516f17a5958/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84= github.com/Microsoft/go-winio v0.4.17/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84= -github.com/Microsoft/go-winio v0.5.0/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84= github.com/Microsoft/go-winio v0.5.1/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84= github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY= github.com/Microsoft/go-winio v0.6.0 h1:slsWYD/zyx7lCXoZVlvQrj0hPTM1HI4+v1sIda2yDvg= @@ -80,9 +77,7 @@ github.com/Microsoft/hcsshim v0.8.14/go.mod h1:NtVKoYxQuTLx6gEq0L96c9Ju4JbRJ4nY2 github.com/Microsoft/hcsshim v0.8.15/go.mod h1:x38A4YbHbdxJtc0sF6oIz+RG0npwSCAvn69iY6URG00= github.com/Microsoft/hcsshim v0.8.16/go.mod h1:o5/SZqmR7x9JNKsW3pu+nqHm0MF8vbA+VxGOoXdC600= github.com/Microsoft/hcsshim v0.8.21/go.mod h1:+w2gRZ5ReXQhFOrvSQeNfhrYB/dg3oDwTOcER2fw4I4= -github.com/Microsoft/hcsshim v0.8.22/go.mod h1:91uVCVzvX2QD16sMCenoxxXo6L1wJnLMX2PSufFMtF0= github.com/Microsoft/hcsshim v0.8.23/go.mod h1:4zegtUJth7lAvFyc6cH2gGQ5B3OFQim01nnU2M8jKDg= -github.com/Microsoft/hcsshim v0.9.4/go.mod h1:7pLA8lDk46WKDWlVsENo92gC0XFa8rbKfyFRBqxEbCc= github.com/Microsoft/hcsshim v0.9.6 h1:VwnDOgLeoi2du6dAznfmspNqTiwczvjv4K7NxuY9jsY= github.com/Microsoft/hcsshim v0.9.6/go.mod h1:7pLA8lDk46WKDWlVsENo92gC0XFa8rbKfyFRBqxEbCc= github.com/Microsoft/hcsshim/test v0.0.0-20201218223536-d3e5debf77da/go.mod h1:5hlzMzRKMLyo42nCZ9oml8AdTlq/0cvIaBv6tK1RehU= @@ -234,8 +229,6 @@ github.com/containerd/nri v0.0.0-20201007170849-eb1350a75164/go.mod h1:+2wGSDGFY github.com/containerd/nri v0.0.0-20210316161719-dbaa18c31c14/go.mod h1:lmxnXF6oMkbqs39FiCt1s0R2HSMhcLel9vNL3m4AaeY= github.com/containerd/nri v0.1.0/go.mod h1:lmxnXF6oMkbqs39FiCt1s0R2HSMhcLel9vNL3m4AaeY= github.com/containerd/stargz-snapshotter/estargz v0.4.1/go.mod h1:x7Q9dg9QYb4+ELgxmo4gBUeJB0tl5dqH1Sdz0nJU1QM= -github.com/containerd/stargz-snapshotter/estargz v0.9.0/go.mod h1:aE5PCyhFMwR8sbrErO5eM2GcvkyXTTJremG883D4qF0= -github.com/containerd/stargz-snapshotter/estargz v0.12.0/go.mod h1:AIQ59TewBFJ4GOPEQXujcrJ/EKxh5xXZegW1rkR1P/M= github.com/containerd/stargz-snapshotter/estargz v0.13.0 h1:fD7AwuVV+B40p0d9qVkH/Au1qhp8hn/HWJHIYjpEcfw= github.com/containerd/stargz-snapshotter/estargz v0.13.0/go.mod h1:m+9VaGJGlhCnrcEUod8mYumTmRgblwd3rC5UCEh2Yp0= github.com/containerd/ttrpc v0.0.0-20190828154514-0e0f228740de/go.mod h1:PvCDdDGpgqzQIzDW1TphrGLssLDZp2GuS+X5DkEJB8o= @@ -279,10 +272,6 @@ github.com/containers/ocicrypt v1.1.6 h1:uoG52u2e91RE4UqmBICZY8dNshgfvkdl3BW6jnx github.com/containers/ocicrypt v1.1.6/go.mod h1:WgjxPWdTJMqYMjf3M6cuIFFA1/MpyyhIM99YInA+Rvc= github.com/containers/psgo v1.8.0 h1:2loGekmGAxM9ir5OsXWEfGwFxorMPYnc6gEDsGFQvhY= github.com/containers/psgo v1.8.0/go.mod h1:T8ZxnX3Ur4RvnhxFJ7t8xJ1F48RhiZB4rSrOaR/qGHc= -github.com/containers/storage v1.37.0/go.mod h1:kqeJeS0b7DO2ZT1nVWs0XufrmPFbgV3c+Q/45RlH6r4= -github.com/containers/storage v1.43.0/go.mod h1:uZ147thiIFGdVTjMmIw19knttQnUCl3y9zjreHrg11s= -github.com/containers/storage v1.44.1-0.20230101110555-a747b27fe4ca h1:vJjfK870nYDzCbFoxaR2hKh/5fFo9LhzgchcXPky2ck= -github.com/containers/storage v1.44.1-0.20230101110555-a747b27fe4ca/go.mod h1:o+bCRUdLbr6MPQaV5TphvdxBUucUBDFqzAcPIi8WWtg= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/go-iptables v0.4.5/go.mod h1:/mVI274lEDI2ns62jHCDnCyBF9Iwsmekav8Dbxlm1MU= @@ -611,12 +600,9 @@ github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= github.com/klauspost/compress v1.11.3/go.mod h1:aoV0uJVorq1K+umq18yTdKaF57EivdYsUV+/s2qKfXs= github.com/klauspost/compress v1.11.13/go.mod h1:aoV0uJVorq1K+umq18yTdKaF57EivdYsUV+/s2qKfXs= -github.com/klauspost/compress v1.13.6/go.mod h1:/3/Vjq9QcHkK5uEr5lBEmyoZ1iFhe47etQ6QUkpK6sk= -github.com/klauspost/compress v1.15.7/go.mod h1:PhcZ0MbTNciWF3rruxRgKxI5NkcHHrHUDtV4Yw2GlzU= -github.com/klauspost/compress v1.15.9/go.mod h1:PhcZ0MbTNciWF3rruxRgKxI5NkcHHrHUDtV4Yw2GlzU= github.com/klauspost/compress v1.15.12/go.mod h1:QPwzmACJjUTFsnSHH934V6woptycfrDDJnH7hvFVbGM= -github.com/klauspost/compress v1.15.13 h1:NFn1Wr8cfnenSJSA46lLq4wHCcBzKTSjnBIexDMMOV0= -github.com/klauspost/compress v1.15.13/go.mod h1:QPwzmACJjUTFsnSHH934V6woptycfrDDJnH7hvFVbGM= +github.com/klauspost/compress v1.15.14 h1:i7WCKDToww0wA+9qrUZ1xOjp218vfFo3nTU6UHp+gOc= +github.com/klauspost/compress v1.15.14/go.mod h1:QPwzmACJjUTFsnSHH934V6woptycfrDDJnH7hvFVbGM= github.com/klauspost/pgzip v1.2.5/go.mod h1:Ch1tH69qFZu15pkjo5kYi6mth2Zzwzt50oCQKQE9RUs= github.com/klauspost/pgzip v1.2.6-0.20220930104621-17e8dac29df8 h1:BcxbplxjtczA1a6d3wYoa7a0WL3rq9DKBMGHeKyjEF0= github.com/klauspost/pgzip v1.2.6-0.20220930104621-17e8dac29df8/go.mod h1:Ch1tH69qFZu15pkjo5kYi6mth2Zzwzt50oCQKQE9RUs= @@ -704,6 +690,8 @@ github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjY github.com/morikuni/aec v1.0.0 h1:nP9CBfwrvYnBRgY6qfDQkygYDmYwOilePFkwzv4dU8A= github.com/morikuni/aec v1.0.0/go.mod h1:BbKIizmSmc5MMPqRYbxO4ZU0S0+P200+tUnFx7PXmsc= github.com/mrunalp/fileutils v0.5.0/go.mod h1:M1WthSahJixYnrXQl/DFQuteStB1weuxD2QJNHXfbSQ= +github.com/mtrmac/storage v0.0.0-20230103232939-ffeaed8fa4f7 h1:fX3wEWtjJYW3vVpQZrtyNrUvTOgB5VLzZWZx8JJKDWQ= +github.com/mtrmac/storage v0.0.0-20230103232939-ffeaed8fa4f7/go.mod h1:OdRUYHrq1HP6iAo79VxqtYuJzC5j4eA2I60jKOoCT7g= github.com/munnerz/goautoneg v0.0.0-20120707110453-a547fc61f48d/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= @@ -768,7 +756,6 @@ github.com/opencontainers/runtime-tools v0.9.1-0.20221014010322-58c91d646d86/go. github.com/opencontainers/selinux v1.6.0/go.mod h1:VVGKuOLlE7v4PJyT6h7mNWvq1rzqiriPsEqVhc+svHE= github.com/opencontainers/selinux v1.8.0/go.mod h1:RScLhm78qiWa2gbVCcGkC7tCGdgk3ogry1nUQF8Evvo= github.com/opencontainers/selinux v1.8.2/go.mod h1:MUIHuUEvKB1wtJjQdOyYRgOnLD2xAPP8dBsCoU0KuF8= -github.com/opencontainers/selinux v1.8.5/go.mod h1:HTvjPFoGMbpQsG886e3lQwnsRWtE4TC1OF3OUvG9FAo= github.com/opencontainers/selinux v1.9.1/go.mod h1:2i0OySw99QjzBBQByd1Gr9gSjvuho1lHsJxIJ3gGbJI= github.com/opencontainers/selinux v1.10.1/go.mod h1:2i0OySw99QjzBBQByd1Gr9gSjvuho1lHsJxIJ3gGbJI= github.com/opencontainers/selinux v1.10.2 h1:NFy2xCsjn7+WspbfZkUd5zyVeisV7VFbPSP96+8/ha4= @@ -928,7 +915,6 @@ github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1 github.com/uber/jaeger-client-go v2.30.0+incompatible h1:D6wyKGCecFaSRUpo8lCVbaOOb6ThwMmTEbhRwtKR97o= github.com/uber/jaeger-client-go v2.30.0+incompatible/go.mod h1:WVhlPFC8FDjOFMMWRy2pZqQJSXxYSwNYOkTr/Z6d3Kk= github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGrc= -github.com/ulikunitz/xz v0.5.10/go.mod h1:nbz6k7qbPmH4IRqmfOplQw/tblSgqTqBwxkY0oWt/14= github.com/ulikunitz/xz v0.5.11 h1:kpFauv27b6ynzBNT/Xy+1k+fK4WswhN/6PN5WhFAGw8= github.com/ulikunitz/xz v0.5.11/go.mod h1:nbz6k7qbPmH4IRqmfOplQw/tblSgqTqBwxkY0oWt/14= github.com/urfave/cli v0.0.0-20171014202726-7bc6a0acffa5/go.mod h1:70zkFmudgCuE/ngEzBv17Jvp/497gISqfk5gWijbERA= @@ -1224,7 +1210,6 @@ golang.org/x/sys v0.0.0-20210426230700-d19ff857e887/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210510120138-977fb7262007/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210616094352-59db8d763f22/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20210820121016-41cdb8703e55/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210906170528-6f6e22806c34/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220114195835-da31bd327af9/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= diff --git a/vendor/github.com/containers/storage/containers.go b/vendor/github.com/containers/storage/containers.go index 872618e68208..106d1d152e04 100644 --- a/vendor/github.com/containers/storage/containers.go +++ b/vendor/github.com/containers/storage/containers.go @@ -141,16 +141,21 @@ type rwContainerStore interface { } type containerStore struct { - lockfile *lockfile.LockFile - dir string - jsonPath [numContainerLocationIndex]string + // The following fields are only set when constructing containerStore, and must never be modified afterwards. + // They are safe to access without any other locking. + lockfile *lockfile.LockFile // Synchronizes readers vs. writers of the _filesystem data_, both cross-process and in-process. + dir string + jsonPath [numContainerLocationIndex]string + + inProcessLock sync.RWMutex // Can _only_ be obtained with lockfile held. + // The following fields can only be read/written with read/write ownership of inProcessLock, respectively. + // Almost all users should use startReading() or startWriting(). lastWrite lockfile.LastWrite containers []*Container idindex *truncindex.TruncIndex byid map[string]*Container bylayer map[string]*Container byname map[string]*Container - loadMut sync.Mutex } func copyContainer(c *Container) *Container { @@ -202,6 +207,7 @@ func (c *Container) MountOpts() []string { } } +// The caller must hold r.inProcessLock for reading. func containerLocation(c *Container) containerLocations { if c.volatileStore { return volatileContainerLocation @@ -216,9 +222,11 @@ func containerLocation(c *Container) containerLocations { // should use startWriting() instead. func (r *containerStore) startWritingWithReload(canReload bool) error { r.lockfile.Lock() + r.inProcessLock.Lock() succeeded := false defer func() { if !succeeded { + r.inProcessLock.Unlock() r.lockfile.Unlock() } }() @@ -241,71 +249,135 @@ func (r *containerStore) startWriting() error { // stopWriting releases locks obtained by startWriting. func (r *containerStore) stopWriting() { + r.inProcessLock.Unlock() r.lockfile.Unlock() } // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). func (r *containerStore) startReading() error { + // inProcessLocked calls the nested function with r.inProcessLock held for writing. + inProcessLocked := func(fn func() error) error { + r.inProcessLock.Lock() + defer r.inProcessLock.Unlock() + return fn() + } + r.lockfile.RLock() - unlockFn := r.lockfile.Unlock // A function to call to clean up, or nil + unlockFn := r.lockfile.Unlock // A function to call to clean up, or nil. defer func() { if unlockFn != nil { unlockFn() } }() + r.inProcessLock.RLock() + unlockFn = r.stopReading - if tryLockedForWriting, err := r.reloadIfChanged(false); err != nil { - if !tryLockedForWriting { - return err - } - unlockFn() - unlockFn = nil - - r.lockfile.Lock() + // If we are lucky, we can just hold the read locks, check that we are fresh, and continue. + _, modified, err := r.modified() + if err != nil { + return err + } + if modified { + // We are unlucky, and need to reload. + // NOTE: Multiple goroutines can get to this place approximately simultaneously. + r.inProcessLock.RUnlock() unlockFn = r.lockfile.Unlock - if _, err := r.reloadIfChanged(true); err != nil { + + // r.lastWrite can change at this point if another goroutine reloads the store before us. That’s why we don’t unconditionally + // trigger a load below; we (lock and) reloadIfChanged() again. + + // First try reloading with r.lockfile held for reading. + // r.inProcessLock will serialize all goroutines that got here; + // each will re-check on-disk state vs. r.lastWrite, and the first one will actually reload the data. + var tryLockedForWriting bool + if err := inProcessLocked(func() error { + // We could optimize this further: The r.lockfile.GetLastWrite() value shouldn’t change as long as we hold r.lockfile, + // so if r.lastWrite was already updated, we don’t need to actually read the on-filesystem lock. + var err error + tryLockedForWriting, err = r.reloadIfChanged(false) return err + }); err != nil { + if !tryLockedForWriting { + return err + } + // Not good enough, we need r.lockfile held for writing. So, let’s do that. + unlockFn() + unlockFn = nil + + r.lockfile.Lock() + unlockFn = r.lockfile.Unlock + if err := inProcessLocked(func() error { + _, err := r.reloadIfChanged(true) + return err + }); err != nil { + return err + } + unlockFn() + unlockFn = nil + + r.lockfile.RLock() + unlockFn = r.lockfile.Unlock + // We need to check for a reload once more because the on-disk state could have been modified + // after we released the lock. + // If that, _again_, finds inconsistent state, just give up. + // We could, plausibly, retry a few times, but that inconsistent state (duplicate container names) + // shouldn’t be saved (by correct implementations) in the first place. + if err := inProcessLocked(func() error { + _, err := r.reloadIfChanged(false) + return err + }); err != nil { + return fmt.Errorf("(even after successfully cleaning up once:) %w", err) + } } - unlockFn() - unlockFn = nil - r.lockfile.RLock() - unlockFn = r.lockfile.Unlock - // We need to check for a reload reload once more because the on-disk state could have been modified - // after we released the lock. - // If that, _again_, finds inconsistent state, just give up. - // We could, plausibly, retry a few times, but that inconsistent state (duplicate container names) - // shouldn’t be saved (by correct implementations) in the first place. - if _, err := r.reloadIfChanged(false); err != nil { - return fmt.Errorf("(even after successfully cleaning up once:) %w", err) - } - } + // NOTE that we hold neither a read nor write inProcessLock at this point. That’s fine in ordinary operation, because + // the on-filesystem r.lockfile should protect us against (cooperating) writers, and any use of r.inProcessLock + // protects us against in-process writers modifying data. + // In presence of non-cooperating writers, we just ensure that 1) the in-memory data is not clearly out-of-date + // and 2) access to the in-memory data is not racy; + // but we can’t protect against those out-of-process writers modifying _files_ while we are assuming they are in a consistent state. + r.inProcessLock.RLock() + } unlockFn = nil return nil } // stopReading releases locks obtained by startReading. func (r *containerStore) stopReading() { + r.inProcessLock.RUnlock() r.lockfile.Unlock() } +// modified returns true if the on-disk state has changed (i.e. if reloadIfChanged may need to modify the store), +// and a lockfile.LastWrite value for that update. +// +// The caller must hold r.lockfile for reading _or_ writing. +// The caller must hold r.inProcessLock for reading or writing. +func (r *containerStore) modified() (lockfile.LastWrite, bool, error) { + return r.lockfile.ModifiedSince(r.lastWrite) +} + // reloadIfChanged reloads the contents of the store from disk if it is changed. // // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true // if it is held for writing. // +// The caller must hold r.inProcessLock for WRITING. +// // If !lockedForWriting and this function fails, the return value indicates whether // reloadIfChanged() with lockedForWriting could succeed. func (r *containerStore) reloadIfChanged(lockedForWriting bool) (bool, error) { - r.loadMut.Lock() - defer r.loadMut.Unlock() - - lastWrite, modified, err := r.lockfile.ModifiedSince(r.lastWrite) + lastWrite, modified, err := r.modified() if err != nil { return false, err } + // We require callers to always hold r.inProcessLock for WRITING, even if they might not end up calling r.load() + // and modify no fields, to ensure they see fresh data: + // r.lockfile.Modified() only returns true once per change. Without an exclusive lock, + // one goroutine might see r.lockfile.Modified() == true and decide to load, and in the meanwhile another one could + // see r.lockfile.Modified() == false and proceed to use in-memory data without noticing it is stale. if modified { if tryLockedForWriting, err := r.load(lockedForWriting); err != nil { return tryLockedForWriting, err // r.lastWrite is unchanged, so we will load the next time again. @@ -315,6 +387,7 @@ func (r *containerStore) reloadIfChanged(lockedForWriting bool) (bool, error) { return false, nil } +// Requires startReading or startWriting. func (r *containerStore) Containers() ([]Container, error) { containers := make([]Container, len(r.containers)) for i := range r.containers { @@ -326,6 +399,7 @@ func (r *containerStore) Containers() ([]Container, error) { // This looks for datadirs in the store directory that are not referenced // by the json file and removes it. These can happen in the case of unclean // shutdowns or regular restarts in transient store mode. +// Requires startReading. func (r *containerStore) GarbageCollect() error { entries, err := os.ReadDir(r.dir) if err != nil { @@ -371,6 +445,7 @@ func (r *containerStore) datapath(id, key string) string { // // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true // if it is held for writing. +// The caller must hold r.inProcessLock for WRITING. // // If !lockedForWriting and this function fails, the return value indicates whether // retrying with lockedForWriting could succeed. @@ -441,8 +516,9 @@ func (r *containerStore) load(lockedForWriting bool) (bool, error) { return false, nil } -// Save saves the contents of the store to disk. It should be called with -// the lock held, locked for writing. +// save saves the contents of the store to disk. +// The caller must hold r.lockfile locked for writing. +// The caller must hold r.inProcessLock for reading (but usually holds it for writing in order to make the desired changes). func (r *containerStore) save(saveLocations containerLocations) error { r.lockfile.AssertLockedForWriting() for locationIndex := 0; locationIndex < numContainerLocationIndex; locationIndex++ { @@ -483,6 +559,9 @@ func (r *containerStore) save(saveLocations containerLocations) error { return nil } +// saveFor saves the contents of the store relevant for modifiedContainer to disk. +// The caller must hold r.lockfile locked for writing. +// The caller must hold r.inProcessLock for reading (but usually holds it for writing in order to make the desired changes). func (r *containerStore) saveFor(modifiedContainer *Container) error { return r.save(containerLocation(modifiedContainer)) } @@ -503,16 +582,17 @@ func newContainerStore(dir string, runDir string, transient bool) (rwContainerSt return nil, err } cstore := containerStore{ - lockfile: lockfile, - dir: dir, - containers: []*Container{}, - byid: make(map[string]*Container), - bylayer: make(map[string]*Container), - byname: make(map[string]*Container), + lockfile: lockfile, + dir: dir, jsonPath: [numContainerLocationIndex]string{ filepath.Join(dir, "containers.json"), filepath.Join(volatileDir, "volatile-containers.json"), }, + + containers: []*Container{}, + byid: make(map[string]*Container), + bylayer: make(map[string]*Container), + byname: make(map[string]*Container), } if err := cstore.startWritingWithReload(false); err != nil { @@ -529,6 +609,7 @@ func newContainerStore(dir string, runDir string, transient bool) (rwContainerSt return &cstore, nil } +// Requires startReading or startWriting. func (r *containerStore) lookup(id string) (*Container, bool) { if container, ok := r.byid[id]; ok { return container, ok @@ -544,6 +625,7 @@ func (r *containerStore) lookup(id string) (*Container, bool) { return nil, false } +// Requires startWriting. func (r *containerStore) ClearFlag(id string, flag string) error { container, ok := r.lookup(id) if !ok { @@ -553,6 +635,7 @@ func (r *containerStore) ClearFlag(id string, flag string) error { return r.saveFor(container) } +// Requires startWriting. func (r *containerStore) SetFlag(id string, flag string, value interface{}) error { container, ok := r.lookup(id) if !ok { @@ -565,6 +648,7 @@ func (r *containerStore) SetFlag(id string, flag string, value interface{}) erro return r.saveFor(container) } +// Requires startWriting. func (r *containerStore) Create(id string, names []string, image, layer, metadata string, options *ContainerOptions) (container *Container, err error) { if id == "" { id = stringid.GenerateRandomID() @@ -624,6 +708,7 @@ func (r *containerStore) Create(id string, names []string, image, layer, metadat return container, err } +// Requires startReading or startWriting. func (r *containerStore) Metadata(id string) (string, error) { if container, ok := r.lookup(id); ok { return container.Metadata, nil @@ -631,6 +716,7 @@ func (r *containerStore) Metadata(id string) (string, error) { return "", ErrContainerUnknown } +// Requires startWriting. func (r *containerStore) SetMetadata(id, metadata string) error { if container, ok := r.lookup(id); ok { container.Metadata = metadata @@ -639,10 +725,12 @@ func (r *containerStore) SetMetadata(id, metadata string) error { return ErrContainerUnknown } +// The caller must hold r.inProcessLock for writing. func (r *containerStore) removeName(container *Container, name string) { container.Names = stringSliceWithoutValue(container.Names, name) } +// Requires startWriting. func (r *containerStore) updateNames(id string, names []string, op updateNameOperation) error { container, ok := r.lookup(id) if !ok { @@ -666,6 +754,7 @@ func (r *containerStore) updateNames(id string, names []string, op updateNameOpe return r.saveFor(container) } +// Requires startWriting. func (r *containerStore) Delete(id string) error { container, ok := r.lookup(id) if !ok { @@ -704,6 +793,7 @@ func (r *containerStore) Delete(id string) error { return nil } +// Requires startReading or startWriting. func (r *containerStore) Get(id string) (*Container, error) { if container, ok := r.lookup(id); ok { return copyContainer(container), nil @@ -711,6 +801,7 @@ func (r *containerStore) Get(id string) (*Container, error) { return nil, ErrContainerUnknown } +// Requires startReading or startWriting. func (r *containerStore) Lookup(name string) (id string, err error) { if container, ok := r.lookup(name); ok { return container.ID, nil @@ -718,11 +809,13 @@ func (r *containerStore) Lookup(name string) (id string, err error) { return "", ErrContainerUnknown } +// Requires startReading or startWriting. func (r *containerStore) Exists(id string) bool { _, ok := r.lookup(id) return ok } +// Requires startReading or startWriting. func (r *containerStore) BigData(id, key string) ([]byte, error) { if key == "" { return nil, fmt.Errorf("can't retrieve container big data value for empty name: %w", ErrInvalidBigDataName) @@ -790,6 +883,7 @@ func (r *containerStore) BigDataDigest(id, key string) (digest.Digest, error) { return "", ErrDigestUnknown } +// Requires startReading or startWriting. func (r *containerStore) BigDataNames(id string) ([]string, error) { c, ok := r.lookup(id) if !ok { @@ -798,6 +892,7 @@ func (r *containerStore) BigDataNames(id string) ([]string, error) { return copyStringSlice(c.BigDataNames), nil } +// Requires startWriting. func (r *containerStore) SetBigData(id, key string, data []byte) error { if key == "" { return fmt.Errorf("can't set empty name for container big data item: %w", ErrInvalidBigDataName) @@ -844,6 +939,7 @@ func (r *containerStore) SetBigData(id, key string, data []byte) error { return err } +// Requires startWriting. func (r *containerStore) Wipe() error { ids := make([]string, 0, len(r.byid)) for id := range r.byid { diff --git a/vendor/github.com/containers/storage/images.go b/vendor/github.com/containers/storage/images.go index 1de0fe59f9e7..577b6f8ed40d 100644 --- a/vendor/github.com/containers/storage/images.go +++ b/vendor/github.com/containers/storage/images.go @@ -157,15 +157,20 @@ type rwImageStore interface { } type imageStore struct { - lockfile *lockfile.LockFile // lockfile.IsReadWrite can be used to distinguish between read-write and read-only image stores. - dir string + // The following fields are only set when constructing imageStore, and must never be modified afterwards. + // They are safe to access without any other locking. + lockfile *lockfile.LockFile // lockfile.IsReadWrite can be used to distinguish between read-write and read-only image stores. + dir string + + inProcessLock sync.RWMutex // Can _only_ be obtained with lockfile held. + // The following fields can only be read/written with read/write ownership of inProcessLock, respectively. + // Almost all users should use startReading() or startWriting(). lastWrite lockfile.LastWrite images []*Image idindex *truncindex.TruncIndex byid map[string]*Image byname map[string]*Image bydigest map[digest.Digest][]*Image - loadMut sync.Mutex } func copyImage(i *Image) *Image { @@ -205,9 +210,11 @@ func copyImageSlice(slice []*Image) []*Image { // should use startReading() instead. func (r *imageStore) startWritingWithReload(canReload bool) error { r.lockfile.Lock() + r.inProcessLock.Lock() succeeded := false defer func() { if !succeeded { + r.inProcessLock.Unlock() r.lockfile.Unlock() } }() @@ -230,6 +237,7 @@ func (r *imageStore) startWriting() error { // stopWriting releases locks obtained by startWriting. func (r *imageStore) stopWriting() { + r.inProcessLock.Unlock() r.lockfile.Unlock() } @@ -239,6 +247,13 @@ func (r *imageStore) stopWriting() { // This is an internal implementation detail of imageStore construction, every other caller // should use startReading() instead. func (r *imageStore) startReadingWithReload(canReload bool) error { + // inProcessLocked calls the nested function with r.inProcessLock held for writing. + inProcessLocked := func(fn func() error) error { + r.inProcessLock.Lock() + defer r.inProcessLock.Unlock() + return fn() + } + r.lockfile.RLock() unlockFn := r.lockfile.Unlock // A function to call to clean up, or nil defer func() { @@ -246,33 +261,76 @@ func (r *imageStore) startReadingWithReload(canReload bool) error { unlockFn() } }() + r.inProcessLock.RLock() + unlockFn = r.stopReading if canReload { - if tryLockedForWriting, err := r.reloadIfChanged(false); err != nil { - if !tryLockedForWriting { - return err - } - unlockFn() - unlockFn = nil - - r.lockfile.Lock() + // If we are lucky, we can just hold the read locks, check that we are fresh, and continue. + _, modified, err := r.modified() + if err != nil { + return err + } + if modified { + // We are unlucky, and need to reload. + // NOTE: Multiple goroutines can get to this place approximately simultaneously. + r.inProcessLock.RUnlock() unlockFn = r.lockfile.Unlock - if _, err := r.reloadIfChanged(true); err != nil { + + // r.lastWrite can change at this point if another goroutine reloads the store before us. That’s why we don’t unconditionally + // trigger a load below; we (lock and) reloadIfChanged() again. + + // First try reloading with r.lockfile held for reading. + // r.inProcessLock will serialize all goroutines that got here; + // each will re-check on-disk state vs. r.lastWrite, and the first one will actually reload the data. + var tryLockedForWriting bool + if err := inProcessLocked(func() error { + // We could optimize this further: The r.lockfile.GetLastWrite() value shouldn’t change as long as we hold r.lockfile, + // so if r.lastWrite was already updated, we don’t need to actually read the on-filesystem lock. + var err error + tryLockedForWriting, err = r.reloadIfChanged(false) return err + }); err != nil { + if !tryLockedForWriting { + return err + } + // Not good enough, we need r.lockfile held for writing. So, let’s do that. + unlockFn() + unlockFn = nil + + r.lockfile.Lock() + unlockFn = r.lockfile.Unlock + if err := inProcessLocked(func() error { + _, err := r.reloadIfChanged(true) + return err + }); err != nil { + return err + } + unlockFn() + unlockFn = nil + + r.lockfile.RLock() + unlockFn = r.lockfile.Unlock + // We need to check for a reload once more because the on-disk state could have been modified + // after we released the lock. + // If that, _again_, finds inconsistent state, just give up. + // We could, plausibly, retry a few times, but that inconsistent state (duplicate image names) + // shouldn’t be saved (by correct implementations) in the first place. + if err := inProcessLocked(func() error { + _, err := r.reloadIfChanged(false) + return err + }); err != nil { + return fmt.Errorf("(even after successfully cleaning up once:) %w", err) + } } - unlockFn() - unlockFn = nil - r.lockfile.RLock() - unlockFn = r.lockfile.Unlock - // We need to check for a reload reload once more because the on-disk state could have been modified - // after we released the lock. - // If that, _again_, finds inconsistent state, just give up. - // We could, plausibly, retry a few times, but that inconsistent state (duplicate image names) - // shouldn’t be saved (by correct implementations) in the first place. - if _, err := r.reloadIfChanged(false); err != nil { - return fmt.Errorf("(even after successfully cleaning up once:) %w", err) - } + // NOTE that we hold neither a read nor write inProcessLock at this point. That’s fine in ordinary operation, because + // the on-filesystem r.lockfile should protect us against (cooperating) writers, and any use of r.inProcessLock + // protects us against in-process writers modifying data. + // In presence of non-cooperating writers, we just ensure that 1) the in-memory data is not clearly out-of-date + // and 2) access to the in-memory data is not racy; + // but we can’t protect against those out-of-process writers modifying _files_ while we are assuming they are in a consistent state. + + r.inProcessLock.RLock() } } @@ -288,24 +346,38 @@ func (r *imageStore) startReading() error { // stopReading releases locks obtained by startReading. func (r *imageStore) stopReading() { + r.inProcessLock.RUnlock() r.lockfile.Unlock() } +// modified returns true if the on-disk state has changed (i.e. if reloadIfChanged may need to modify the store), +// and a lockfile.LastWrite value for that update. +// +// The caller must hold r.lockfile for reading _or_ writing. +// The caller must hold r.inProcessLock for reading or writing. +func (r *imageStore) modified() (lockfile.LastWrite, bool, error) { + return r.lockfile.ModifiedSince(r.lastWrite) +} + // reloadIfChanged reloads the contents of the store from disk if it is changed. // // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true // if it is held for writing. // +// The caller must hold r.inProcessLock for WRITING. +// // If !lockedForWriting and this function fails, the return value indicates whether // reloadIfChanged() with lockedForWriting could succeed. func (r *imageStore) reloadIfChanged(lockedForWriting bool) (bool, error) { - r.loadMut.Lock() - defer r.loadMut.Unlock() - - lastWrite, modified, err := r.lockfile.ModifiedSince(r.lastWrite) + lastWrite, modified, err := r.modified() if err != nil { return false, err } + // We require callers to always hold r.inProcessLock for WRITING, even if they might not end up calling r.load() + // and modify no fields, to ensure they see fresh data: + // r.lockfile.Modified() only returns true once per change. Without an exclusive lock, + // one goroutine might see r.lockfile.Modified() == true and decide to load, and in the meanwhile another one could + // see r.lockfile.Modified() == false and proceed to use in-memory data without noticing it is stale. if modified { if tryLockedForWriting, err := r.load(lockedForWriting); err != nil { return tryLockedForWriting, err // r.lastWrite is unchanged, so we will load the next time again. @@ -315,6 +387,7 @@ func (r *imageStore) reloadIfChanged(lockedForWriting bool) (bool, error) { return false, nil } +// Requires startReading or startWriting. func (r *imageStore) Images() ([]Image, error) { images := make([]Image, len(r.images)) for i := range r.images { @@ -345,6 +418,7 @@ func bigDataNameIsManifest(name string) bool { // recomputeDigests takes a fixed digest and a name-to-digest map and builds a // list of the unique values that would identify the image. +// The caller must hold r.inProcessLock for writing. func (i *Image) recomputeDigests() error { validDigests := make([]digest.Digest, 0, len(i.BigDataDigests)+1) digests := make(map[digest.Digest]struct{}) @@ -382,6 +456,7 @@ func (i *Image) recomputeDigests() error { // // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true // if it is held for writing. +// The caller must hold r.inProcessLock for WRITING. // // If !lockedForWriting and this function fails, the return value indicates whether // retrying with lockedForWriting could succeed. @@ -445,8 +520,9 @@ func (r *imageStore) load(lockedForWriting bool) (bool, error) { return false, nil } -// Save saves the contents of the store to disk. It should be called with -// the lock held, locked for writing. +// Save saves the contents of the store to disk. +// The caller must hold r.lockfile locked for writing. +// The caller must hold r.inProcessLock for reading (but usually holds it for writing in order to make the desired changes). func (r *imageStore) Save() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the image store at %q: %w", r.imagespath(), ErrStoreIsReadOnly) @@ -482,6 +558,7 @@ func newImageStore(dir string) (rwImageStore, error) { istore := imageStore{ lockfile: lockfile, dir: dir, + images: []*Image{}, byid: make(map[string]*Image), byname: make(map[string]*Image), @@ -509,6 +586,7 @@ func newROImageStore(dir string) (roImageStore, error) { istore := imageStore{ lockfile: lockfile, dir: dir, + images: []*Image{}, byid: make(map[string]*Image), byname: make(map[string]*Image), @@ -528,6 +606,7 @@ func newROImageStore(dir string) (roImageStore, error) { return &istore, nil } +// Requires startReading or startWriting. func (r *imageStore) lookup(id string) (*Image, bool) { if image, ok := r.byid[id]; ok { return image, ok @@ -540,6 +619,7 @@ func (r *imageStore) lookup(id string) (*Image, bool) { return nil, false } +// Requires startWriting. func (r *imageStore) ClearFlag(id string, flag string) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to clear flags on images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) @@ -552,6 +632,7 @@ func (r *imageStore) ClearFlag(id string, flag string) error { return r.Save() } +// Requires startWriting. func (r *imageStore) SetFlag(id string, flag string, value interface{}) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to set flags on images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) @@ -567,6 +648,7 @@ func (r *imageStore) SetFlag(id string, flag string, value interface{}) error { return r.Save() } +// Requires startWriting. func (r *imageStore) Create(id string, names []string, layer, metadata string, created time.Time, searchableDigest digest.Digest) (image *Image, err error) { if !r.lockfile.IsReadWrite() { return nil, fmt.Errorf("not allowed to create new images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) @@ -626,6 +708,7 @@ func (r *imageStore) Create(id string, names []string, layer, metadata string, c return image, err } +// Requires startWriting. func (r *imageStore) addMappedTopLayer(id, layer string) error { if image, ok := r.lookup(id); ok { image.MappedTopLayers = append(image.MappedTopLayers, layer) @@ -634,6 +717,7 @@ func (r *imageStore) addMappedTopLayer(id, layer string) error { return fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } +// Requires startWriting. func (r *imageStore) removeMappedTopLayer(id, layer string) error { if image, ok := r.lookup(id); ok { initialLen := len(image.MappedTopLayers) @@ -647,6 +731,7 @@ func (r *imageStore) removeMappedTopLayer(id, layer string) error { return fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } +// Requires startReading or startWriting. func (r *imageStore) Metadata(id string) (string, error) { if image, ok := r.lookup(id); ok { return image.Metadata, nil @@ -654,6 +739,7 @@ func (r *imageStore) Metadata(id string) (string, error) { return "", fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } +// Requires startWriting. func (r *imageStore) SetMetadata(id, metadata string) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify image metadata at %q: %w", r.imagespath(), ErrStoreIsReadOnly) @@ -665,14 +751,17 @@ func (r *imageStore) SetMetadata(id, metadata string) error { return fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } +// The caller must hold r.inProcessLock for writing. func (r *imageStore) removeName(image *Image, name string) { image.Names = stringSliceWithoutValue(image.Names, name) } +// The caller must hold r.inProcessLock for writing. func (i *Image) addNameToHistory(name string) { i.NamesHistory = dedupeNames(append([]string{name}, i.NamesHistory...)) } +// Requires startWriting. func (r *imageStore) updateNames(id string, names []string, op updateNameOperation) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to change image name assignments at %q: %w", r.imagespath(), ErrStoreIsReadOnly) @@ -700,6 +789,7 @@ func (r *imageStore) updateNames(id string, names []string, op updateNameOperati return r.Save() } +// Requires startWriting. func (r *imageStore) Delete(id string) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) @@ -747,6 +837,7 @@ func (r *imageStore) Delete(id string) error { return nil } +// Requires startReading or startWriting. func (r *imageStore) Get(id string) (*Image, error) { if image, ok := r.lookup(id); ok { return copyImage(image), nil @@ -754,11 +845,13 @@ func (r *imageStore) Get(id string) (*Image, error) { return nil, fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } +// Requires startReading or startWriting. func (r *imageStore) Exists(id string) bool { _, ok := r.lookup(id) return ok } +// Requires startReading or startWriting. func (r *imageStore) ByDigest(d digest.Digest) ([]*Image, error) { if images, ok := r.bydigest[d]; ok { return copyImageSlice(images), nil @@ -766,6 +859,7 @@ func (r *imageStore) ByDigest(d digest.Digest) ([]*Image, error) { return nil, fmt.Errorf("locating image with digest %q: %w", d, ErrImageUnknown) } +// Requires startReading or startWriting. func (r *imageStore) BigData(id, key string) ([]byte, error) { if key == "" { return nil, fmt.Errorf("can't retrieve image big data value for empty name: %w", ErrInvalidBigDataName) @@ -777,6 +871,7 @@ func (r *imageStore) BigData(id, key string) ([]byte, error) { return os.ReadFile(r.datapath(image.ID, key)) } +// Requires startReading or startWriting. func (r *imageStore) BigDataSize(id, key string) (int64, error) { if key == "" { return -1, fmt.Errorf("can't retrieve size of image big data with empty name: %w", ErrInvalidBigDataName) @@ -794,6 +889,7 @@ func (r *imageStore) BigDataSize(id, key string) (int64, error) { return -1, ErrSizeUnknown } +// Requires startReading or startWriting. func (r *imageStore) BigDataDigest(id, key string) (digest.Digest, error) { if key == "" { return "", fmt.Errorf("can't retrieve digest of image big data value with empty name: %w", ErrInvalidBigDataName) @@ -808,6 +904,7 @@ func (r *imageStore) BigDataDigest(id, key string) (digest.Digest, error) { return "", ErrDigestUnknown } +// Requires startReading or startWriting. func (r *imageStore) BigDataNames(id string) ([]string, error) { image, ok := r.lookup(id) if !ok { @@ -827,6 +924,7 @@ func imageSliceWithoutValue(slice []*Image, value *Image) []*Image { return modified } +// Requires startWriting. func (r *imageStore) SetBigData(id, key string, data []byte, digestManifest func([]byte) (digest.Digest, error)) error { if key == "" { return fmt.Errorf("can't set empty name for image big data item: %w", ErrInvalidBigDataName) @@ -911,6 +1009,7 @@ func (r *imageStore) SetBigData(id, key string, data []byte, digestManifest func return err } +// Requires startWriting. func (r *imageStore) Wipe() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) diff --git a/vendor/github.com/containers/storage/layers.go b/vendor/github.com/containers/storage/layers.go index 5dcd2ff161d7..f14108be5aa6 100644 --- a/vendor/github.com/containers/storage/layers.go +++ b/vendor/github.com/containers/storage/layers.go @@ -83,13 +83,26 @@ type Layer struct { MountLabel string `json:"mountlabel,omitempty"` // MountPoint is the path where the layer is mounted, or where it was most - // recently mounted. This can change between subsequent Unmount() and - // Mount() calls, so the caller should consult this value after Mount() - // succeeds to find the location of the container's root filesystem. + // recently mounted. + // + // WARNING: This field is a snapshot in time: (except for users inside c/storage that + // hold the mount lock) the true value can change between subsequent + // calls to c/storage API. + // + // Users that need to handle concurrent mount/unmount attempts should not access this + // field at all, and should only use the path returned by .Mount() (and that’s only + // assuming no other user will concurrently decide to unmount that mount point). MountPoint string `json:"-"` // MountCount is used as a reference count for the container's layer being // mounted at the mount point. + // + // WARNING: This field is a snapshot in time; (except for users inside c/storage that + // hold the mount lock) the true value can change between subsequent + // calls to c/storage API. + // + // In situations where concurrent mount/unmount attempts can happen, this field + // should not be used for any decisions, maybe apart from heuristic user warnings. MountCount int `json:"-"` // Created is the datestamp for when this layer was created. Older @@ -265,8 +278,15 @@ type rwLayerStore interface { // The mappings used by the container can be specified. Mount(id string, options drivers.MountOpts) (string, error) - // Unmount unmounts a layer when it is no longer in use. - Unmount(id string, force bool) (bool, error) + // unmount unmounts a layer when it is no longer in use. + // If conditional is set, it will fail with ErrLayerNotMounted if the layer is not mounted (without conditional, the caller is + // making a promise that the layer is actually mounted). + // If force is set, it will physically try to unmount it even if it is mounted multple times, or even if (!conditional and) + // there are no records of it being mounted in the first place. + // It returns whether the layer was still mounted at the time this function returned. + // WARNING: The return value may already be obsolete by the time it is available + // to the caller, so it can be used for heuristic sanity checks at best. It should almost always be ignored. + unmount(id string, force bool, conditional bool) (bool, error) // Mounted returns number of times the layer has been mounted. Mounted(id string) (int, error) @@ -305,12 +325,17 @@ type rwLayerStore interface { } type layerStore struct { - lockfile *lockfile.LockFile - mountsLockfile *lockfile.LockFile - rundir string - jsonPath [numLayerLocationIndex]string - driver drivers.Driver - layerdir string + // The following fields are only set when constructing layerStore, and must never be modified afterwards. + // They are safe to access without any other locking. + lockfile *lockfile.LockFile // lockfile.IsReadWrite can be used to distinguish between read-write and read-only layer stores. + mountsLockfile *lockfile.LockFile // Can _only_ be obtained with inProcessLock held. + rundir string + jsonPath [numLayerLocationIndex]string + layerdir string + + inProcessLock sync.RWMutex // Can _only_ be obtained with lockfile held. + // The following fields can only be read/written with read/write ownership of inProcessLock, respectively. + // Almost all users should use startReading() or startWriting(). lastWrite lockfile.LastWrite mountsLastWrite lockfile.LastWrite // Only valid if lockfile.IsReadWrite() layers []*Layer @@ -320,10 +345,14 @@ type layerStore struct { bymount map[string]*Layer bycompressedsum map[digest.Digest][]string byuncompressedsum map[digest.Digest][]string - loadMut sync.Mutex layerspathsModified [numLayerLocationIndex]time.Time + + // FIXME: This field is only set when constructing layerStore, but locking rules of the driver + // interface itself are not documented here. + driver drivers.Driver } +// The caller must hold r.inProcessLock for reading. func layerLocation(l *Layer) layerLocations { if l.volatileStore { return volatileLayerLocation @@ -364,9 +393,11 @@ func copyLayer(l *Layer) *Layer { // should use startWriting() instead. func (r *layerStore) startWritingWithReload(canReload bool) error { r.lockfile.Lock() + r.inProcessLock.Lock() succeeded := false defer func() { if !succeeded { + r.inProcessLock.Unlock() r.lockfile.Unlock() } }() @@ -389,6 +420,7 @@ func (r *layerStore) startWriting() error { // stopWriting releases locks obtained by startWriting. func (r *layerStore) stopWriting() { + r.inProcessLock.Unlock() r.lockfile.Unlock() } @@ -398,6 +430,13 @@ func (r *layerStore) stopWriting() { // This is an internal implementation detail of layerStore construction, every other caller // should use startReading() instead. func (r *layerStore) startReadingWithReload(canReload bool) error { + // inProcessLocked calls the nested function with r.inProcessLock held for writing. + inProcessLocked := func(fn func() error) error { + r.inProcessLock.Lock() + defer r.inProcessLock.Unlock() + return fn() + } + r.lockfile.RLock() unlockFn := r.lockfile.Unlock // A function to call to clean up, or nil defer func() { @@ -405,36 +444,71 @@ func (r *layerStore) startReadingWithReload(canReload bool) error { unlockFn() } }() + r.inProcessLock.RLock() + unlockFn = r.stopReading if canReload { - cleanupsDone := 0 - for { - tryLockedForWriting, err := r.reloadIfChanged(false) - if err == nil { - break - } - if !tryLockedForWriting { - return err - } - if cleanupsDone >= maxLayerStoreCleanupIterations { - return fmt.Errorf("(even after %d cleanup attempts:) %w", cleanupsDone, err) - } - unlockFn() - unlockFn = nil - - r.lockfile.Lock() + // If we are lucky, we can just hold the read locks, check that we are fresh, and continue. + modified, err := r.modified() + if err != nil { + return err + } + if modified { + // We are unlucky, and need to reload. + // NOTE: Multiple goroutines can get to this place approximately simultaneously. + r.inProcessLock.RUnlock() unlockFn = r.lockfile.Unlock - if _, err := r.reloadIfChanged(true); err != nil { - return err + + cleanupsDone := 0 + for { + // First try reloading with r.lockfile held for reading. + // r.inProcessLock will serialize all goroutines that got here; + // each will re-check on-disk state vs. r.lastWrite, and the first one will actually reload the data. + var tryLockedForWriting bool + err := inProcessLocked(func() error { + var err error + tryLockedForWriting, err = r.reloadIfChanged(false) + return err + }) + if err == nil { + break + } + if !tryLockedForWriting { + return err + } + if cleanupsDone >= maxLayerStoreCleanupIterations { + return fmt.Errorf("(even after %d cleanup attempts:) %w", cleanupsDone, err) + } + // Not good enough, we need r.lockfile held for writing. So, let’s do that. + unlockFn() + unlockFn = nil + + r.lockfile.Lock() + unlockFn = r.lockfile.Unlock + if err := inProcessLocked(func() error { + _, err := r.reloadIfChanged(true) + return err + }); err != nil { + return err + } + unlockFn() + unlockFn = nil + + r.lockfile.RLock() + unlockFn = r.lockfile.Unlock + // We need to check for a reload again because the on-disk state could have been modified + // after we released the lock. + cleanupsDone++ } - unlockFn() - unlockFn = nil - r.lockfile.RLock() - unlockFn = r.lockfile.Unlock - // We need to check for a reload reload again because the on-disk state could have been modified - // after we released the lock. - cleanupsDone++ + // NOTE that we hold neither a read nor write inProcessLock at this point. That’s fine in ordinary operation, because + // the on-filesystem r.lockfile should protect us against (cooperating) writers, and any use of r.inProcessLock + // protects us against in-process writers modifying data. + // In presence of non-cooperating writers, we just ensure that 1) the in-memory data is not clearly out-of-date + // and 2) access to the in-memory data is not racy; + // but we can’t protect against those out-of-process writers modifying _files_ while we are assuming they are in a consistent state. + + r.inProcessLock.RLock() } } @@ -450,13 +524,48 @@ func (r *layerStore) startReading() error { // stopReading releases locks obtained by startReading. func (r *layerStore) stopReading() { + r.inProcessLock.RUnlock() r.lockfile.Unlock() } +// modified returns true if the on-disk state (of layers or mounts) has changed (ie if reloadIcHanged may need to modify the store) +// +// Note that unlike containerStore.modified and imageStore.modified, this function is not directly used in layerStore.reloadIfChanged(); +// it exists only to help the reader ensure it has fresh enough state. +// +// The caller must hold r.lockfile for reading _or_ writing. +// The caller must hold r.inProcessLock for reading or writing. +func (r *layerStore) modified() (bool, error) { + _, m, err := r.layersModified() + if err != nil { + return false, err + } + if m { + return true, nil + } + if r.lockfile.IsReadWrite() { + // This means we get, release, and re-obtain, r.mountsLockfile if we actually need to do any kind of reload. + // That’s a bit expensive, but hopefully most callers will be read-only and see no changes. + // We can’t eliminate these mountsLockfile accesses given the current assumption that Layer objects have _some_ not-very-obsolete + // mount data. Maybe we can segregate the mount-dependent and mount-independent operations better... + r.mountsLockfile.RLock() + defer r.mountsLockfile.Unlock() + _, m, err := r.mountsModified() + if err != nil { + return false, err + } + if m { + return true, nil + } + } + return false, nil +} + // layersModified() checks if the most recent writer to r.jsonPath[] was a party other than the // last recorded writer. If so, it returns a lockfile.LastWrite value to record on a successful // reload. // It should only be called with the lock held. +// The caller must hold r.inProcessLock for reading. func (r *layerStore) layersModified() (lockfile.LastWrite, bool, error) { lastWrite, modified, err := r.lockfile.ModifiedSince(r.lastWrite) if err != nil { @@ -488,12 +597,11 @@ func (r *layerStore) layersModified() (lockfile.LastWrite, bool, error) { // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true // if it is held for writing. // +// The caller must hold r.inProcessLock for WRITING. +// // If !lockedForWriting and this function fails, the return value indicates whether // reloadIfChanged() with lockedForWriting could succeed. func (r *layerStore) reloadIfChanged(lockedForWriting bool) (bool, error) { - r.loadMut.Lock() - defer r.loadMut.Unlock() - lastWrite, layersModified, err := r.layersModified() if err != nil { return false, err @@ -516,11 +624,20 @@ func (r *layerStore) reloadIfChanged(lockedForWriting bool) (bool, error) { return false, nil } +// mountsModified returns true if the on-disk mount state has changed (i.e. if reloadMountsIfChanged may need to modify the store), +// and a lockfile.LastWrite value for that update. +// +// The caller must hold r.mountsLockfile for reading _or_ writing. +// The caller must hold r.inProcessLock for reading or writing. +func (r *layerStore) mountsModified() (lockfile.LastWrite, bool, error) { + return r.mountsLockfile.ModifiedSince(r.mountsLastWrite) +} + // reloadMountsIfChanged reloads the contents of mountsPath from disk if it is changed. // // The caller must hold r.mountsLockFile for reading or writing. func (r *layerStore) reloadMountsIfChanged() error { - lastWrite, modified, err := r.mountsLockfile.ModifiedSince(r.mountsLastWrite) + lastWrite, modified, err := r.mountsModified() if err != nil { return err } @@ -533,6 +650,7 @@ func (r *layerStore) reloadMountsIfChanged() error { return nil } +// Requires startReading or startWriting. func (r *layerStore) Layers() ([]Layer, error) { layers := make([]Layer, len(r.layers)) for i := range r.layers { @@ -541,6 +659,7 @@ func (r *layerStore) Layers() ([]Layer, error) { return layers, nil } +// Requires startWriting. func (r *layerStore) GarbageCollect() error { layers, err := r.driver.ListLayers() @@ -581,6 +700,7 @@ func (r *layerStore) mountspath() string { // // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true // if it is held for writing. +// The caller must hold r.inProcessLock for WRITING. // // If !lockedForWriting and this function fails, the return value indicates whether // retrying with lockedForWriting could succeed. @@ -694,6 +814,11 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) { return false, err } r.mountsLastWrite = mountsLastWrite + // NOTE: We will release mountsLockfile when this function returns, so unlike most of the layer data, the + // r.layers[].MountPoint, r.layers[].MountCount, and r.bymount values might not reflect + // true on-filesystem state already by the time this function returns. + // Code that needs the state to be accurate must lock r.mountsLockfile again, + // and possibly loadMounts() again. } if errorToResolveBySaving != nil { @@ -731,6 +856,8 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) { return false, nil } +// The caller must hold r.mountsLockfile for reading or writing. +// The caller must hold r.inProcessLock for WRITING. func (r *layerStore) loadMounts() error { mounts := make(map[string]*Layer) mpath := r.mountspath() @@ -768,8 +895,9 @@ func (r *layerStore) loadMounts() error { return err } -// Save saves the contents of the store to disk. It should be called with -// the lock held, locked for writing. +// save saves the contents of the store to disk. +// The caller must hold r.lockfile locked for writing. +// The caller must hold r.inProcessLock for WRITING. func (r *layerStore) save(saveLocations layerLocations) error { r.mountsLockfile.Lock() defer r.mountsLockfile.Unlock() @@ -779,10 +907,15 @@ func (r *layerStore) save(saveLocations layerLocations) error { return r.saveMounts() } +// saveFor saves the contents of the store relevant for modifiedLayer to disk. +// The caller must hold r.lockfile locked for writing. +// The caller must hold r.inProcessLock for WRITING. func (r *layerStore) saveFor(modifiedLayer *Layer) error { return r.save(layerLocation(modifiedLayer)) } +// The caller must hold r.lockfile locked for writing. +// The caller must hold r.inProcessLock for WRITING. func (r *layerStore) saveLayers(saveLocations layerLocations) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -826,6 +959,8 @@ func (r *layerStore) saveLayers(saveLocations layerLocations) error { return nil } +// The caller must hold r.mountsLockfile for writing. +// The caller must hold r.inProcessLock for WRITING. func (r *layerStore) saveMounts() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -887,16 +1022,18 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri rlstore := layerStore{ lockfile: lockFile, mountsLockfile: mountsLockfile, - driver: driver, rundir: rundir, - layerdir: layerdir, - byid: make(map[string]*Layer), - bymount: make(map[string]*Layer), - byname: make(map[string]*Layer), jsonPath: [numLayerLocationIndex]string{ filepath.Join(layerdir, "layers.json"), filepath.Join(volatileDir, "volatile-layers.json"), }, + layerdir: layerdir, + + byid: make(map[string]*Layer), + byname: make(map[string]*Layer), + bymount: make(map[string]*Layer), + + driver: driver, } if err := rlstore.startWritingWithReload(false); err != nil { return nil, err @@ -922,16 +1059,18 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL rlstore := layerStore{ lockfile: lockfile, mountsLockfile: nil, - driver: driver, rundir: rundir, - layerdir: layerdir, - byid: make(map[string]*Layer), - bymount: make(map[string]*Layer), - byname: make(map[string]*Layer), jsonPath: [numLayerLocationIndex]string{ filepath.Join(layerdir, "layers.json"), filepath.Join(layerdir, "volatile-layers.json"), }, + layerdir: layerdir, + + byid: make(map[string]*Layer), + byname: make(map[string]*Layer), + bymount: make(map[string]*Layer), + + driver: driver, } if err := rlstore.startReadingWithReload(false); err != nil { return nil, err @@ -948,6 +1087,7 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL return &rlstore, nil } +// Requires startReading or startWriting. func (r *layerStore) lookup(id string) (*Layer, bool) { if layer, ok := r.byid[id]; ok { return layer, ok @@ -960,6 +1100,7 @@ func (r *layerStore) lookup(id string) (*Layer, bool) { return nil, false } +// Requires startReading or startWriting. func (r *layerStore) Size(name string) (int64, error) { layer, ok := r.lookup(name) if !ok { @@ -974,6 +1115,7 @@ func (r *layerStore) Size(name string) (int64, error) { return -1, nil } +// Requires startWriting. func (r *layerStore) ClearFlag(id string, flag string) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to clear flags on layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -986,6 +1128,7 @@ func (r *layerStore) ClearFlag(id string, flag string) error { return r.saveFor(layer) } +// Requires startWriting. func (r *layerStore) SetFlag(id string, flag string, value interface{}) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to set flags on layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -1005,6 +1148,7 @@ func (r *layerStore) Status() ([][2]string, error) { return r.driver.Status(), nil } +// Requires startWriting. func (r *layerStore) PutAdditionalLayer(id string, parentLayer *Layer, names []string, aLayer drivers.AdditionalLayer) (layer *Layer, err error) { if duplicateLayer, idInUse := r.byid[id]; idInUse { return duplicateLayer, ErrDuplicateID @@ -1061,6 +1205,7 @@ func (r *layerStore) PutAdditionalLayer(id string, parentLayer *Layer, names []s return copyLayer(layer), nil } +// Requires startWriting. func (r *layerStore) Put(id string, parentLayer *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, flags map[string]interface{}, diff io.Reader) (*Layer, int64, error) { if !r.lockfile.IsReadWrite() { return nil, -1, fmt.Errorf("not allowed to create new layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -1258,32 +1403,41 @@ func (r *layerStore) Put(id string, parentLayer *Layer, names []string, mountLab return layer, size, err } +// Requires startWriting. func (r *layerStore) CreateWithFlags(id string, parent *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, flags map[string]interface{}) (layer *Layer, err error) { layer, _, err = r.Put(id, parent, names, mountLabel, options, moreOptions, writeable, flags, nil) return layer, err } +// Requires startWriting. func (r *layerStore) Create(id string, parent *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool) (layer *Layer, err error) { return r.CreateWithFlags(id, parent, names, mountLabel, options, moreOptions, writeable, nil) } +// Requires startReading or startWriting. func (r *layerStore) Mounted(id string) (int, error) { if !r.lockfile.IsReadWrite() { return 0, fmt.Errorf("no mount information for layers at %q: %w", r.mountspath(), ErrStoreIsReadOnly) } - r.mountsLockfile.RLock() - defer r.mountsLockfile.Unlock() - if err := r.reloadMountsIfChanged(); err != nil { - return 0, err - } layer, ok := r.lookup(id) if !ok { return 0, ErrLayerUnknown } + // NOTE: The caller of this function is not holding (currently cannot hold) r.mountsLockfile, + // so the data is necessarily obsolete by the time this function returns. So, we don’t even + // try to reload it in this function, we just rely on r.load() that happened during + // r.startReading() or r.startWriting(). return layer.MountCount, nil } +// Requires startWriting. func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error) { + // LOCKING BUG: This is reachable via store.Diff → layerStore.Diff → layerStore.newFileGetter + // (with btrfs and zfs graph drivers) holding layerStore only locked for reading, while it modifies + // - r.layers[].MountCount (directly and via loadMounts / saveMounts) + // - r.layers[].MountPoint (directly and via loadMounts / saveMounts) + // - r.bymount (via loadMounts / saveMounts) + // check whether options include ro option hasReadOnlyOpt := func(opts []string) bool { for _, item := range opts { @@ -1343,7 +1497,14 @@ func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error) return mountpoint, err } -func (r *layerStore) Unmount(id string, force bool) (bool, error) { +// Requires startWriting. +func (r *layerStore) unmount(id string, force bool, conditional bool) (bool, error) { + // LOCKING BUG: This is reachable via store.Diff → layerStore.Diff → layerStore.newFileGetter → simpleGetCloser.Close() + // (with btrfs and zfs graph drivers) holding layerStore only locked for reading, while it modifies + // - r.layers[].MountCount (directly and via loadMounts / saveMounts) + // - r.layers[].MountPoint (directly and via loadMounts / saveMounts) + // - r.bymount (via loadMounts / saveMounts) + if !r.lockfile.IsReadWrite() { return false, fmt.Errorf("not allowed to update mount locations for layers at %q: %w", r.mountspath(), ErrStoreIsReadOnly) } @@ -1360,6 +1521,9 @@ func (r *layerStore) Unmount(id string, force bool) (bool, error) { } layer = layerByMount } + if conditional && layer.MountCount == 0 { + return false, ErrLayerNotMounted + } if force { layer.MountCount = 1 } @@ -1379,15 +1543,16 @@ func (r *layerStore) Unmount(id string, force bool) (bool, error) { return true, err } +// Requires startReading or startWriting. func (r *layerStore) ParentOwners(id string) (uids, gids []int, err error) { if !r.lockfile.IsReadWrite() { return nil, nil, fmt.Errorf("no mount information for layers at %q: %w", r.mountspath(), ErrStoreIsReadOnly) } r.mountsLockfile.RLock() defer r.mountsLockfile.Unlock() - if err := r.reloadMountsIfChanged(); err != nil { - return nil, nil, err - } + // We are not checking r.mountsLockfile.Modified() and calling r.loadMounts here because the store + // is only locked for reading = we are not allowed to modify layer data. + // Holding r.mountsLockfile protects us against concurrent mount/unmount operations. layer, ok := r.lookup(id) if !ok { return nil, nil, ErrLayerUnknown @@ -1400,6 +1565,13 @@ func (r *layerStore) ParentOwners(id string) (uids, gids []int, err error) { // We don't know which directories to examine. return nil, nil, ErrLayerNotMounted } + // Holding r.mountsLockfile protects us against concurrent mount/unmount operations, but we didn’t + // hold it continuously since the time we loaded the mount data; so it’s possible the layer + // was unmounted in the meantime, or mounted elsewhere. Treat that as if we were run after the unmount, + // = a missing mount, not a filesystem error. + if _, err := system.Lstat(layer.MountPoint); errors.Is(err, os.ErrNotExist) { + return nil, nil, ErrLayerNotMounted + } rootuid, rootgid, err := idtools.GetRootUIDGID(layer.UIDMap, layer.GIDMap) if err != nil { return nil, nil, fmt.Errorf("reading root ID values for layer %q: %w", layer.ID, err) @@ -1448,10 +1620,12 @@ func (r *layerStore) ParentOwners(id string) (uids, gids []int, err error) { return uids, gids, nil } +// Requires startWriting. func (r *layerStore) removeName(layer *Layer, name string) { layer.Names = stringSliceWithoutValue(layer.Names, name) } +// Requires startWriting. func (r *layerStore) updateNames(id string, names []string, op updateNameOperation) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to change layer name assignments at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -1486,6 +1660,7 @@ func (r *layerStore) datapath(id, key string) string { return filepath.Join(r.datadir(id), makeBigDataBaseName(key)) } +// Requires startReading or startWriting. func (r *layerStore) BigData(id, key string) (io.ReadCloser, error) { if key == "" { return nil, fmt.Errorf("can't retrieve layer big data value for empty name: %w", ErrInvalidBigDataName) @@ -1497,6 +1672,7 @@ func (r *layerStore) BigData(id, key string) (io.ReadCloser, error) { return os.Open(r.datapath(layer.ID, key)) } +// Requires startWriting. func (r *layerStore) SetBigData(id, key string, data io.Reader) error { if key == "" { return fmt.Errorf("can't set empty name for layer big data item: %w", ErrInvalidBigDataName) @@ -1544,6 +1720,7 @@ func (r *layerStore) SetBigData(id, key string, data io.Reader) error { return nil } +// Requires startReading or startWriting. func (r *layerStore) BigDataNames(id string) ([]string, error) { layer, ok := r.lookup(id) if !ok { @@ -1552,6 +1729,7 @@ func (r *layerStore) BigDataNames(id string) ([]string, error) { return copyStringSlice(layer.BigDataNames), nil } +// Requires startReading or startWriting. func (r *layerStore) Metadata(id string) (string, error) { if layer, ok := r.lookup(id); ok { return layer.Metadata, nil @@ -1559,6 +1737,7 @@ func (r *layerStore) Metadata(id string) (string, error) { return "", ErrLayerUnknown } +// Requires startWriting. func (r *layerStore) SetMetadata(id, metadata string) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify layer metadata at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -1575,6 +1754,7 @@ func (r *layerStore) tspath(id string) string { } // layerHasIncompleteFlag returns true if layer.Flags contains an incompleteFlag set to true +// The caller must hold r.inProcessLock for reading. func layerHasIncompleteFlag(layer *Layer) bool { // layer.Flags[…] is defined to succeed and return ok == false if Flags == nil if flagValue, ok := layer.Flags[incompleteFlag]; ok { @@ -1585,6 +1765,7 @@ func layerHasIncompleteFlag(layer *Layer) bool { return false } +// Requires startWriting. func (r *layerStore) deleteInternal(id string) error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -1655,6 +1836,7 @@ func (r *layerStore) deleteInternal(id string) error { return nil } +// Requires startWriting. func (r *layerStore) deleteInDigestMap(id string) { for digest, layers := range r.bycompressedsum { for i, layerID := range layers { @@ -1676,6 +1858,7 @@ func (r *layerStore) deleteInDigestMap(id string) { } } +// Requires startWriting. func (r *layerStore) Delete(id string) error { layer, ok := r.lookup(id) if !ok { @@ -1685,17 +1868,13 @@ func (r *layerStore) Delete(id string) error { // The layer may already have been explicitly unmounted, but if not, we // should try to clean that up before we start deleting anything at the // driver level. - mountCount, err := r.Mounted(id) - if err != nil { - return fmt.Errorf("checking if layer %q is still mounted: %w", id, err) - } - for mountCount > 0 { - if _, err := r.Unmount(id, false); err != nil { - return err + for { + _, err := r.unmount(id, false, true) + if err == ErrLayerNotMounted { + break } - mountCount, err = r.Mounted(id) if err != nil { - return fmt.Errorf("checking if layer %q is still mounted: %w", id, err) + return err } } if err := r.deleteInternal(id); err != nil { @@ -1704,11 +1883,13 @@ func (r *layerStore) Delete(id string) error { return r.saveFor(layer) } +// Requires startReading or startWriting. func (r *layerStore) Exists(id string) bool { _, ok := r.lookup(id) return ok } +// Requires startReading or startWriting. func (r *layerStore) Get(id string) (*Layer, error) { if layer, ok := r.lookup(id); ok { return copyLayer(layer), nil @@ -1716,6 +1897,7 @@ func (r *layerStore) Get(id string) (*Layer, error) { return nil, ErrLayerUnknown } +// Requires startWriting. func (r *layerStore) Wipe() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete layers at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -1735,6 +1917,7 @@ func (r *layerStore) Wipe() error { return nil } +// Requires startReading or startWriting. func (r *layerStore) findParentAndLayer(from, to string) (fromID string, toID string, fromLayer, toLayer *Layer, err error) { var ok bool toLayer, ok = r.lookup(to) @@ -1759,6 +1942,7 @@ func (r *layerStore) findParentAndLayer(from, to string) (fromID string, toID st return from, to, fromLayer, toLayer, nil } +// The caller must hold r.inProcessLock for reading. func (r *layerStore) layerMappings(layer *Layer) *idtools.IDMappings { if layer == nil { return &idtools.IDMappings{} @@ -1766,6 +1950,7 @@ func (r *layerStore) layerMappings(layer *Layer) *idtools.IDMappings { return idtools.NewIDMappingsFromMaps(layer.UIDMap, layer.GIDMap) } +// Requires startReading or startWriting. func (r *layerStore) Changes(from, to string) ([]archive.Change, error) { from, to, fromLayer, toLayer, err := r.findParentAndLayer(from, to) if err != nil { @@ -1784,11 +1969,13 @@ func (s *simpleGetCloser) Get(path string) (io.ReadCloser, error) { return os.Open(filepath.Join(s.path, path)) } +// LOCKING BUG: See the comments in layerStore.Diff func (s *simpleGetCloser) Close() error { - _, err := s.r.Unmount(s.id, false) + _, err := s.r.unmount(s.id, false, false) return err } +// LOCKING BUG: See the comments in layerStore.Diff func (r *layerStore) newFileGetter(id string) (drivers.FileGetCloser, error) { if getter, ok := r.driver.(drivers.DiffGetterDriver); ok { return getter.DiffGetter(id) @@ -1822,6 +2009,7 @@ func writeCompressedDataGoroutine(pwriter *io.PipeWriter, compressor io.WriteClo err = writeCompressedData(compressor, source) } +// Requires startReading or startWriting. func (r *layerStore) Diff(from, to string, options *DiffOptions) (io.ReadCloser, error) { var metadata storage.Unpacker @@ -1933,6 +2121,8 @@ func (r *layerStore) Diff(from, to string, options *DiffOptions) (io.ReadCloser, metadata = storage.NewJSONUnpacker(decompressor) + // LOCKING BUG: With btrfs and zfs graph drivers), this uses r.Mount() and r.unmount() holding layerStore only locked for reading + // but they modify in-memory state. fgetter, err := r.newFileGetter(to) if err != nil { errs := multierror.Append(nil, fmt.Errorf("creating file-getter: %w", err)) @@ -1968,6 +2158,7 @@ func (r *layerStore) Diff(from, to string, options *DiffOptions) (io.ReadCloser, return maybeCompressReadCloser(rc) } +// Requires startReading or startWriting. func (r *layerStore) DiffSize(from, to string) (size int64, err error) { var fromLayer, toLayer *Layer from, to, fromLayer, toLayer, err = r.findParentAndLayer(from, to) @@ -1977,10 +2168,12 @@ func (r *layerStore) DiffSize(from, to string) (size int64, err error) { return r.driver.DiffSize(to, r.layerMappings(toLayer), from, r.layerMappings(fromLayer), toLayer.MountLabel) } +// Requires startWriting. func (r *layerStore) ApplyDiff(to string, diff io.Reader) (size int64, err error) { return r.applyDiffWithOptions(to, nil, diff) } +// Requires startWriting. func (r *layerStore) applyDiffWithOptions(to string, layerOptions *LayerOptions, diff io.Reader) (size int64, err error) { if !r.lockfile.IsReadWrite() { return -1, fmt.Errorf("not allowed to modify layer contents at %q: %w", r.layerdir, ErrStoreIsReadOnly) @@ -2127,6 +2320,7 @@ func (r *layerStore) applyDiffWithOptions(to string, layerOptions *LayerOptions, return size, err } +// Requires (startReading or?) startWriting. func (r *layerStore) DifferTarget(id string) (string, error) { ddriver, ok := r.driver.(drivers.DriverWithDiffer) if !ok { @@ -2139,6 +2333,7 @@ func (r *layerStore) DifferTarget(id string) (string, error) { return ddriver.DifferTarget(layer.ID) } +// Requires startWriting. func (r *layerStore) ApplyDiffFromStagingDirectory(id, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffOpts) error { ddriver, ok := r.driver.(drivers.DriverWithDiffer) if !ok { @@ -2177,6 +2372,7 @@ func (r *layerStore) ApplyDiffFromStagingDirectory(id, stagingDirectory string, return err } +// Requires startWriting. func (r *layerStore) ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) { ddriver, ok := r.driver.(drivers.DriverWithDiffer) if !ok { @@ -2216,6 +2412,7 @@ func (r *layerStore) CleanupStagingDirectory(stagingDirectory string) error { return ddriver.CleanupStagingDirectory(stagingDirectory) } +// Requires startReading or startWriting. func (r *layerStore) layersByDigestMap(m map[digest.Digest][]string, d digest.Digest) ([]Layer, error) { var layers []Layer for _, layerID := range m[d] { @@ -2228,10 +2425,12 @@ func (r *layerStore) layersByDigestMap(m map[digest.Digest][]string, d digest.Di return layers, nil } +// Requires startReading or startWriting. func (r *layerStore) LayersByCompressedDigest(d digest.Digest) ([]Layer, error) { return r.layersByDigestMap(r.bycompressedsum, d) } +// Requires startReading or startWriting. func (r *layerStore) LayersByUncompressedDigest(d digest.Digest) ([]Layer, error) { return r.layersByDigestMap(r.byuncompressedsum, d) } diff --git a/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go b/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go index b654699b9549..ec25f8a9cfeb 100644 --- a/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go +++ b/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go @@ -49,7 +49,7 @@ type Locker interface { // It might do nothing at all, or it may panic if the caller is not the owner of this lock. AssertLocked() - // AssertLocked() can be used by callers that _know_ that they hold the lock locked for writing, for sanity checking. + // AssertLockedForWriting() can be used by callers that _know_ that they hold the lock locked for writing, for sanity checking. // It might do nothing at all, or it may panic if the caller is not the owner of this lock for writing. AssertLockedForWriting() } diff --git a/vendor/github.com/containers/storage/store.go b/vendor/github.com/containers/storage/store.go index f819722ba94d..8407f6922e49 100644 --- a/vendor/github.com/containers/storage/store.go +++ b/vendor/github.com/containers/storage/store.go @@ -247,6 +247,8 @@ type Store interface { // Unmount attempts to unmount an image, given an ID. // Returns whether or not the layer is still mounted. + // WARNING: The return value may already be obsolete by the time it is available + // to the caller, so it can be used for heuristic sanity checks at best. It should almost always be ignored. UnmountImage(id string, force bool) (bool, error) // Mount attempts to mount a layer, image, or container for access, and @@ -265,9 +267,15 @@ type Store interface { // Unmount attempts to unmount a layer, image, or container, given an ID, a // name, or a mount path. Returns whether or not the layer is still mounted. + // WARNING: The return value may already be obsolete by the time it is available + // to the caller, so it can be used for heuristic sanity checks at best. It should almost always be ignored. Unmount(id string, force bool) (bool, error) // Mounted returns number of times the layer has been mounted. + // + // WARNING: This value might already be obsolete by the time it is returned; + // In situations where concurrent mount/unmount attempts can happen, this field + // should not be used for any decisions, maybe apart from heuristic user warnings. Mounted(id string) (int, error) // Changes returns a summary of the changes which would need to be made @@ -2588,7 +2596,7 @@ func (s *store) Unmount(id string, force bool) (bool, error) { err := s.writeToLayerStore(func(rlstore rwLayerStore) error { if rlstore.Exists(id) { var err error - res, err = rlstore.Unmount(id, force) + res, err = rlstore.unmount(id, force, false) return err } return ErrLayerUnknown @@ -3149,8 +3157,11 @@ func (s *store) Shutdown(force bool) ([]string, error) { } mounted = append(mounted, layer.ID) if force { - for layer.MountCount > 0 { - _, err2 := rlstore.Unmount(layer.ID, force) + for { + _, err2 := rlstore.unmount(layer.ID, force, true) + if err2 == ErrLayerNotMounted { + break + } if err2 != nil { if err == nil { err = err2 diff --git a/vendor/github.com/containers/storage/userns.go b/vendor/github.com/containers/storage/userns.go index 4d3a9d75cc2a..720752f83759 100644 --- a/vendor/github.com/containers/storage/userns.go +++ b/vendor/github.com/containers/storage/userns.go @@ -197,7 +197,7 @@ outer: return 0, err } defer func() { - if _, err2 := rlstore.Unmount(clayer.ID, true); err2 != nil { + if _, err2 := rlstore.unmount(clayer.ID, true, false); err2 != nil { if retErr == nil { retErr = fmt.Errorf("unmounting temporary layer %#v: %w", clayer.ID, err2) } else { diff --git a/vendor/github.com/klauspost/compress/.goreleaser.yml b/vendor/github.com/klauspost/compress/.goreleaser.yml index 0af08e65e682..a2bf06e94f61 100644 --- a/vendor/github.com/klauspost/compress/.goreleaser.yml +++ b/vendor/github.com/klauspost/compress/.goreleaser.yml @@ -3,7 +3,7 @@ before: hooks: - ./gen.sh - - go install mvdan.cc/garble@latest + - go install mvdan.cc/garble@v0.7.2 builds: - diff --git a/vendor/github.com/klauspost/compress/README.md b/vendor/github.com/klauspost/compress/README.md index 7469b40fa428..d73fb86e4fb8 100644 --- a/vendor/github.com/klauspost/compress/README.md +++ b/vendor/github.com/klauspost/compress/README.md @@ -9,7 +9,6 @@ This package provides various compression algorithms. * [huff0](https://github.com/klauspost/compress/tree/master/huff0) and [FSE](https://github.com/klauspost/compress/tree/master/fse) implementations for raw entropy encoding. * [gzhttp](https://github.com/klauspost/compress/tree/master/gzhttp) Provides client and server wrappers for handling gzipped requests efficiently. * [pgzip](https://github.com/klauspost/pgzip) is a separate package that provides a very fast parallel gzip implementation. -* [fuzz package](https://github.com/klauspost/compress-fuzz) for fuzz testing all compressors/decompressors here. [![Go Reference](https://pkg.go.dev/badge/klauspost/compress.svg)](https://pkg.go.dev/github.com/klauspost/compress?tab=subdirectories) [![Go](https://github.com/klauspost/compress/actions/workflows/go.yml/badge.svg)](https://github.com/klauspost/compress/actions/workflows/go.yml) @@ -17,6 +16,10 @@ This package provides various compression algorithms. # changelog +* Dec 11, 2022 (v1.15.13) + * zstd: Add [MaxEncodedSize](https://pkg.go.dev/github.com/klauspost/compress@v1.15.13/zstd#Encoder.MaxEncodedSize) to encoder https://github.com/klauspost/compress/pull/691 + * zstd: Various tweaks and improvements https://github.com/klauspost/compress/pull/693 https://github.com/klauspost/compress/pull/695 https://github.com/klauspost/compress/pull/696 https://github.com/klauspost/compress/pull/701 https://github.com/klauspost/compress/pull/702 https://github.com/klauspost/compress/pull/703 https://github.com/klauspost/compress/pull/704 https://github.com/klauspost/compress/pull/705 https://github.com/klauspost/compress/pull/706 https://github.com/klauspost/compress/pull/707 https://github.com/klauspost/compress/pull/708 + * Oct 26, 2022 (v1.15.12) * zstd: Tweak decoder allocs. https://github.com/klauspost/compress/pull/680 diff --git a/vendor/github.com/klauspost/compress/flate/stateless.go b/vendor/github.com/klauspost/compress/flate/stateless.go index 93a1d150312e..f3d4139ef362 100644 --- a/vendor/github.com/klauspost/compress/flate/stateless.go +++ b/vendor/github.com/klauspost/compress/flate/stateless.go @@ -86,11 +86,19 @@ func StatelessDeflate(out io.Writer, in []byte, eof bool, dict []byte) error { dict = dict[len(dict)-maxStatelessDict:] } + // For subsequent loops, keep shallow dict reference to avoid alloc+copy. + var inDict []byte + for len(in) > 0 { todo := in - if len(todo) > maxStatelessBlock-len(dict) { + if len(inDict) > 0 { + if len(todo) > maxStatelessBlock-maxStatelessDict { + todo = todo[:maxStatelessBlock-maxStatelessDict] + } + } else if len(todo) > maxStatelessBlock-len(dict) { todo = todo[:maxStatelessBlock-len(dict)] } + inOrg := in in = in[len(todo):] uncompressed := todo if len(dict) > 0 { @@ -102,7 +110,11 @@ func StatelessDeflate(out io.Writer, in []byte, eof bool, dict []byte) error { todo = combined } // Compress - statelessEnc(&dst, todo, int16(len(dict))) + if len(inDict) == 0 { + statelessEnc(&dst, todo, int16(len(dict))) + } else { + statelessEnc(&dst, inDict[:maxStatelessDict+len(todo)], maxStatelessDict) + } isEof := eof && len(in) == 0 if dst.n == 0 { @@ -119,7 +131,8 @@ func StatelessDeflate(out io.Writer, in []byte, eof bool, dict []byte) error { } if len(in) > 0 { // Retain a dict if we have more - dict = todo[len(todo)-maxStatelessDict:] + inDict = inOrg[len(uncompressed)-maxStatelessDict:] + dict = nil dst.Reset() } if bw.err != nil { diff --git a/vendor/github.com/klauspost/compress/zstd/decodeheader.go b/vendor/github.com/klauspost/compress/zstd/decodeheader.go index 1f5d41bf415f..f6a240970d46 100644 --- a/vendor/github.com/klauspost/compress/zstd/decodeheader.go +++ b/vendor/github.com/klauspost/compress/zstd/decodeheader.go @@ -152,7 +152,7 @@ func (h *Header) Decode(in []byte) error { } b, in = in[:size], in[size:] h.HeaderSize += int(size) - switch size { + switch len(b) { case 1: h.DictionaryID = uint32(b[0]) case 2: @@ -182,7 +182,7 @@ func (h *Header) Decode(in []byte) error { } b, in = in[:fcsSize], in[fcsSize:] h.HeaderSize += int(fcsSize) - switch fcsSize { + switch len(b) { case 1: h.FrameContentSize = uint64(b[0]) case 2: diff --git a/vendor/github.com/klauspost/compress/zstd/framedec.go b/vendor/github.com/klauspost/compress/zstd/framedec.go index 6cb3b4e55f8e..65984bf07ca6 100644 --- a/vendor/github.com/klauspost/compress/zstd/framedec.go +++ b/vendor/github.com/klauspost/compress/zstd/framedec.go @@ -167,7 +167,7 @@ func (d *frameDec) reset(br byteBuffer) error { return err } var id uint32 - switch size { + switch len(b) { case 1: id = uint32(b[0]) case 2: @@ -204,7 +204,7 @@ func (d *frameDec) reset(br byteBuffer) error { println("Reading Frame content", err) return err } - switch fcsSize { + switch len(b) { case 1: d.FrameContentSize = uint64(b[0]) case 2: diff --git a/vendor/github.com/klauspost/compress/zstd/seqdec_amd64.s b/vendor/github.com/klauspost/compress/zstd/seqdec_amd64.s index 52e5703c26c4..b94993a07278 100644 --- a/vendor/github.com/klauspost/compress/zstd/seqdec_amd64.s +++ b/vendor/github.com/klauspost/compress/zstd/seqdec_amd64.s @@ -320,10 +320,6 @@ error_not_enough_literals: MOVQ $0x00000004, ret+24(FP) RET - // Return with not enough output space error - MOVQ $0x00000005, ret+24(FP) - RET - // func sequenceDecs_decode_56_amd64(s *sequenceDecs, br *bitReader, ctx *decodeAsmContext) int // Requires: CMOV TEXT ·sequenceDecs_decode_56_amd64(SB), $8-32 @@ -617,10 +613,6 @@ error_not_enough_literals: MOVQ $0x00000004, ret+24(FP) RET - // Return with not enough output space error - MOVQ $0x00000005, ret+24(FP) - RET - // func sequenceDecs_decode_bmi2(s *sequenceDecs, br *bitReader, ctx *decodeAsmContext) int // Requires: BMI, BMI2, CMOV TEXT ·sequenceDecs_decode_bmi2(SB), $8-32 @@ -897,10 +889,6 @@ error_not_enough_literals: MOVQ $0x00000004, ret+24(FP) RET - // Return with not enough output space error - MOVQ $0x00000005, ret+24(FP) - RET - // func sequenceDecs_decode_56_bmi2(s *sequenceDecs, br *bitReader, ctx *decodeAsmContext) int // Requires: BMI, BMI2, CMOV TEXT ·sequenceDecs_decode_56_bmi2(SB), $8-32 @@ -1152,10 +1140,6 @@ error_not_enough_literals: MOVQ $0x00000004, ret+24(FP) RET - // Return with not enough output space error - MOVQ $0x00000005, ret+24(FP) - RET - // func sequenceDecs_executeSimple_amd64(ctx *executeAsmContext) bool // Requires: SSE TEXT ·sequenceDecs_executeSimple_amd64(SB), $8-9 @@ -1389,8 +1373,7 @@ loop_finished: MOVQ ctx+0(FP), AX MOVQ DX, 24(AX) MOVQ DI, 104(AX) - MOVQ 80(AX), CX - SUBQ CX, SI + SUBQ 80(AX), SI MOVQ SI, 112(AX) RET @@ -1402,8 +1385,7 @@ error_match_off_too_big: MOVQ ctx+0(FP), AX MOVQ DX, 24(AX) MOVQ DI, 104(AX) - MOVQ 80(AX), CX - SUBQ CX, SI + SUBQ 80(AX), SI MOVQ SI, 112(AX) RET @@ -1747,8 +1729,7 @@ loop_finished: MOVQ ctx+0(FP), AX MOVQ DX, 24(AX) MOVQ DI, 104(AX) - MOVQ 80(AX), CX - SUBQ CX, SI + SUBQ 80(AX), SI MOVQ SI, 112(AX) RET @@ -1760,8 +1741,7 @@ error_match_off_too_big: MOVQ ctx+0(FP), AX MOVQ DX, 24(AX) MOVQ DI, 104(AX) - MOVQ 80(AX), CX - SUBQ CX, SI + SUBQ 80(AX), SI MOVQ SI, 112(AX) RET diff --git a/vendor/modules.txt b/vendor/modules.txt index b0a9694d108d..6c38e4fd00c6 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -264,7 +264,7 @@ github.com/containers/psgo/internal/dev github.com/containers/psgo/internal/host github.com/containers/psgo/internal/proc github.com/containers/psgo/internal/process -# github.com/containers/storage v1.44.1-0.20230101110555-a747b27fe4ca +# github.com/containers/storage v1.44.1-0.20230101110555-a747b27fe4ca => github.com/mtrmac/storage v0.0.0-20230103232939-ffeaed8fa4f7 ## explicit; go 1.17 github.com/containers/storage github.com/containers/storage/drivers @@ -483,7 +483,7 @@ github.com/jinzhu/copier # github.com/json-iterator/go v1.1.12 ## explicit; go 1.12 github.com/json-iterator/go -# github.com/klauspost/compress v1.15.13 +# github.com/klauspost/compress v1.15.14 ## explicit; go 1.17 github.com/klauspost/compress github.com/klauspost/compress/flate @@ -971,3 +971,4 @@ gopkg.in/yaml.v3 ## explicit; go 1.12 sigs.k8s.io/yaml # github.com/opencontainers/runc => github.com/opencontainers/runc v1.1.1-0.20220617142545-8b9452f75cbc +# github.com/containers/storage => github.com/mtrmac/storage v0.0.0-20230103232939-ffeaed8fa4f7