From 5e8b3253d5d957746cecd680a7ceb9c861cce35c Mon Sep 17 00:00:00 2001 From: a Date: Thu, 15 Aug 2024 09:06:24 -0500 Subject: [PATCH 1/6] noot --- account_test.go | 34 ++++---- memorystorage.go | 186 ++++++++++++++++++++++++++++++++++++++++++ memorystorage_test.go | 72 ++++++++++++++++ 3 files changed, 275 insertions(+), 17 deletions(-) create mode 100644 memorystorage.go create mode 100644 memorystorage_test.go diff --git a/account_test.go b/account_test.go index 45697ece..2504d1b0 100644 --- a/account_test.go +++ b/account_test.go @@ -29,17 +29,17 @@ import ( "go.uber.org/zap" ) -// memoryStorage is an in-memory storage implementation with known contents *and* fixed iteration order for List. -type memoryStorage struct { - contents []memoryStorageItem +// testingMemoryStorage is an in-memory storage implementation with known contents *and* fixed iteration order for List. +type testingMemoryStorage struct { + contents []testingMemoryStorageItem } -type memoryStorageItem struct { +type testingMemoryStorageItem struct { key string data []byte } -func (m *memoryStorage) lookup(_ context.Context, key string) *memoryStorageItem { +func (m *testingMemoryStorage) lookup(_ context.Context, key string) *testingMemoryStorageItem { for _, item := range m.contents { if item.key == key { return &item @@ -47,7 +47,7 @@ func (m *memoryStorage) lookup(_ context.Context, key string) *memoryStorageItem } return nil } -func (m *memoryStorage) Delete(ctx context.Context, key string) error { +func (m *testingMemoryStorage) Delete(ctx context.Context, key string) error { for i, item := range m.contents { if item.key == key { m.contents = append(m.contents[:i], m.contents[i+1:]...) @@ -56,14 +56,14 @@ func (m *memoryStorage) Delete(ctx context.Context, key string) error { } return fs.ErrNotExist } -func (m *memoryStorage) Store(ctx context.Context, key string, value []byte) error { - m.contents = append(m.contents, memoryStorageItem{key: key, data: value}) +func (m *testingMemoryStorage) Store(ctx context.Context, key string, value []byte) error { + m.contents = append(m.contents, testingMemoryStorageItem{key: key, data: value}) return nil } -func (m *memoryStorage) Exists(ctx context.Context, key string) bool { +func (m *testingMemoryStorage) Exists(ctx context.Context, key string) bool { return m.lookup(ctx, key) != nil } -func (m *memoryStorage) List(ctx context.Context, path string, recursive bool) ([]string, error) { +func (m *testingMemoryStorage) List(ctx context.Context, path string, recursive bool) ([]string, error) { if recursive { panic("unimplemented") } @@ -88,22 +88,22 @@ nextitem: } return result, nil } -func (m *memoryStorage) Load(ctx context.Context, key string) ([]byte, error) { +func (m *testingMemoryStorage) Load(ctx context.Context, key string) ([]byte, error) { if item := m.lookup(ctx, key); item != nil { return item.data, nil } return nil, fs.ErrNotExist } -func (m *memoryStorage) Stat(ctx context.Context, key string) (KeyInfo, error) { +func (m *testingMemoryStorage) Stat(ctx context.Context, key string) (KeyInfo, error) { if item := m.lookup(ctx, key); item != nil { return KeyInfo{Key: key, Size: int64(len(item.data))}, nil } return KeyInfo{}, fs.ErrNotExist } -func (m *memoryStorage) Lock(ctx context.Context, name string) error { panic("unimplemented") } -func (m *memoryStorage) Unlock(ctx context.Context, name string) error { panic("unimplemented") } +func (m *testingMemoryStorage) Lock(ctx context.Context, name string) error { panic("unimplemented") } +func (m *testingMemoryStorage) Unlock(ctx context.Context, name string) error { panic("unimplemented") } -var _ Storage = (*memoryStorage)(nil) +var _ Storage = (*testingMemoryStorage)(nil) type recordingStorage struct { Storage @@ -293,7 +293,7 @@ func TestGetAccountAlreadyExistsSkipsBroken(t *testing.T) { am := &ACMEIssuer{CA: dummyCA, Logger: zap.NewNop(), mu: new(sync.Mutex)} testConfig := &Config{ Issuers: []Issuer{am}, - Storage: &memoryStorage{}, + Storage: &testingMemoryStorage{}, Logger: defaultTestLogger, certCache: new(Cache), } @@ -342,7 +342,7 @@ func TestGetAccountWithEmailAlreadyExists(t *testing.T) { am := &ACMEIssuer{CA: dummyCA, Logger: zap.NewNop(), mu: new(sync.Mutex)} testConfig := &Config{ Issuers: []Issuer{am}, - Storage: &recordingStorage{Storage: &memoryStorage{}}, + Storage: &recordingStorage{Storage: &testingMemoryStorage{}}, Logger: defaultTestLogger, certCache: new(Cache), } diff --git a/memorystorage.go b/memorystorage.go new file mode 100644 index 00000000..0d44c8b7 --- /dev/null +++ b/memorystorage.go @@ -0,0 +1,186 @@ +// Copyright 2015 Matthew Holt +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package certmagic + +import ( + "context" + "os" + "path" + "strings" + "sync" + "time" + + "golang.org/x/sync/semaphore" +) + +type storageEntry struct { + i KeyInfo + d []byte +} + +// memoryStorage is a Storage implemention that exists only in memory +// it is intended for testing and one-time-deploys where no persistence is needed +type memoryStorage struct { + m map[string]storageEntry + mu sync.RWMutex + + kmu *keyMutex +} + +func NewMemoryStorage() Storage { + return &memoryStorage{ + m: map[string]storageEntry{}, + kmu: newKeyMutex(), + } +} + +// Exists returns true if key exists in s. +func (s *memoryStorage) Exists(ctx context.Context, key string) bool { + ans, err := s.List(ctx, key, true) + if err != nil { + return false + } + return len(ans) != 0 +} + +// Store saves value at key. +func (s *memoryStorage) Store(_ context.Context, key string, value []byte) error { + s.m[key] = storageEntry{ + i: KeyInfo{ + Key: key, + Modified: time.Now(), + Size: int64(len(value)), + IsTerminal: true, + }, + d: value, + } + return nil +} + +// Load retrieves the value at key. +func (s *memoryStorage) Load(_ context.Context, key string) ([]byte, error) { + val, ok := s.m[key] + if !ok { + return nil, os.ErrNotExist + } + return val.d, nil +} + +// Delete deletes the value at key. +func (s *memoryStorage) Delete(_ context.Context, key string) error { + delete(s.m, key) + return nil +} + +// List returns all keys that match prefix. +func (s *memoryStorage) List(ctx context.Context, prefix string, recursive bool) ([]string, error) { + var keyList []string + + keys := make([]string, 0, len(s.m)) + for k := range s.m { + if strings.HasPrefix(k, prefix) { + keys = append(keys, k) + } + } + // adapted from https://github.com/pberkel/caddy-storage-redis/blob/main/storage.go#L369 + // Iterate over each child key + for _, k := range keys { + // Directory keys will have a "/" suffix + trimmedKey := strings.TrimSuffix(k, "/") + // Reconstruct the full path of child key + fullPathKey := path.Join(prefix, trimmedKey) + // If current key is a directory + if recursive && k != trimmedKey { + // Recursively traverse all child directories + childKeys, err := s.List(ctx, fullPathKey, recursive) + if err != nil { + return keyList, err + } + keyList = append(keyList, childKeys...) + } else { + keyList = append(keyList, fullPathKey) + } + } + + return keys, nil +} + +// Stat returns information about key. +func (s *memoryStorage) Stat(_ context.Context, key string) (KeyInfo, error) { + val, ok := s.m[key] + if !ok { + return KeyInfo{}, os.ErrNotExist + } + return val.i, nil +} + +// Lock obtains a lock named by the given name. It blocks +// until the lock can be obtained or an error is returned. +func (s *memoryStorage) Lock(ctx context.Context, name string) error { + return s.kmu.LockKey(ctx, name) +} + +// Unlock releases the lock for name. +func (s *memoryStorage) Unlock(_ context.Context, name string) error { + return s.kmu.UnlockKey(name) +} + +func (s *memoryStorage) String() string { + return "memoryStorage" +} + +// Interface guard +var _ Storage = (*memoryStorage)(nil) + +type keyMutex struct { + m map[string]*semaphore.Weighted + mu sync.Mutex +} + +func newKeyMutex() *keyMutex { + return &keyMutex{ + m: map[string]*semaphore.Weighted{}, + } +} + +func (km *keyMutex) LockKey(ctx context.Context, id string) error { + select { + case <-ctx.Done(): + // as a special case, caddy allows for the cancelled context to be used for a trylock. + if km.mutex(id).TryAcquire(1) { + return nil + } + return ctx.Err() + default: + return km.mutex(id).Acquire(ctx, 1) + } +} + +// Releases the lock associated with the specified ID. +func (km *keyMutex) UnlockKey(id string) error { + km.mutex(id).Release(1) + return nil +} + +func (km *keyMutex) mutex(id string) *semaphore.Weighted { + km.mu.Lock() + defer km.mu.Unlock() + val, ok := km.m[id] + if !ok { + val = semaphore.NewWeighted(1) + km.m[id] = val + } + return val +} diff --git a/memorystorage_test.go b/memorystorage_test.go new file mode 100644 index 00000000..a12e8f77 --- /dev/null +++ b/memorystorage_test.go @@ -0,0 +1,72 @@ +package certmagic_test + +import ( + "bytes" + "context" + "os" + "testing" + + "github.com/caddyserver/certmagic" + "github.com/caddyserver/certmagic/internal/testutil" +) + +func TestMemoryStorageStoreLoad(t *testing.T) { + ctx := context.Background() + tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*") + testutil.RequireNoError(t, err, "allocating tmp dir") + defer os.RemoveAll(tmpDir) + s := certmagic.NewMemoryStorage() + err = s.Store(ctx, "foo", []byte("bar")) + testutil.RequireNoError(t, err) + dat, err := s.Load(ctx, "foo") + testutil.RequireNoError(t, err) + testutil.RequireEqualValues(t, dat, []byte("bar")) +} + +func TestMemoryStorageStoreLoadRace(t *testing.T) { + ctx := context.Background() + tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*") + testutil.RequireNoError(t, err, "allocating tmp dir") + defer os.RemoveAll(tmpDir) + s := certmagic.NewMemoryStorage() + a := bytes.Repeat([]byte("a"), 4096*1024) + b := bytes.Repeat([]byte("b"), 4096*1024) + err = s.Store(ctx, "foo", a) + testutil.RequireNoError(t, err) + done := make(chan struct{}) + go func() { + err := s.Store(ctx, "foo", b) + testutil.RequireNoError(t, err) + close(done) + }() + dat, err := s.Load(ctx, "foo") + <-done + testutil.RequireNoError(t, err) + testutil.RequireEqualValues(t, 4096*1024, len(dat)) +} + +func TestMemoryStorageWriteLock(t *testing.T) { + ctx := context.Background() + tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*") + testutil.RequireNoError(t, err, "allocating tmp dir") + defer os.RemoveAll(tmpDir) + s := certmagic.NewMemoryStorage() + // cctx is a cancelled ctx. so if we can't immediately get the lock, it will fail + cctx, cn := context.WithCancel(ctx) + cn() + // should success + err = s.Lock(cctx, "foo") + testutil.RequireNoError(t, err) + // should fail + err = s.Lock(cctx, "foo") + testutil.RequireError(t, err) + + err = s.Unlock(cctx, "foo") + testutil.RequireNoError(t, err) + // shouldn't fail + err = s.Lock(cctx, "foo") + testutil.RequireNoError(t, err) + + err = s.Unlock(cctx, "foo") + testutil.RequireNoError(t, err) +} From 67375d2da386b17166733e02215e18ab6811b407 Mon Sep 17 00:00:00 2001 From: a Date: Fri, 28 Mar 2025 14:54:24 -0500 Subject: [PATCH 2/6] use atomicfile to create the file --- account_test.go | 34 ++++---- filestorage.go | 33 ++++---- memorystorage.go | 186 ------------------------------------------ memorystorage_test.go | 72 ---------------- 4 files changed, 32 insertions(+), 293 deletions(-) delete mode 100644 memorystorage.go delete mode 100644 memorystorage_test.go diff --git a/account_test.go b/account_test.go index b1dc1281..dfb77bd4 100644 --- a/account_test.go +++ b/account_test.go @@ -29,17 +29,17 @@ import ( "go.uber.org/zap" ) -// testingMemoryStorage is an in-memory storage implementation with known contents *and* fixed iteration order for List. -type testingMemoryStorage struct { - contents []testingMemoryStorageItem +// memoryStorage is an in-memory storage implementation with known contents *and* fixed iteration order for List. +type memoryStorage struct { + contents []memoryStorageItem } -type testingMemoryStorageItem struct { +type memoryStorageItem struct { key string data []byte } -func (m *testingMemoryStorage) lookup(_ context.Context, key string) *testingMemoryStorageItem { +func (m *memoryStorage) lookup(_ context.Context, key string) *memoryStorageItem { for _, item := range m.contents { if item.key == key { return &item @@ -47,7 +47,7 @@ func (m *testingMemoryStorage) lookup(_ context.Context, key string) *testingMem } return nil } -func (m *testingMemoryStorage) Delete(ctx context.Context, key string) error { +func (m *memoryStorage) Delete(ctx context.Context, key string) error { for i, item := range m.contents { if item.key == key { m.contents = append(m.contents[:i], m.contents[i+1:]...) @@ -56,14 +56,14 @@ func (m *testingMemoryStorage) Delete(ctx context.Context, key string) error { } return fs.ErrNotExist } -func (m *testingMemoryStorage) Store(ctx context.Context, key string, value []byte) error { - m.contents = append(m.contents, testingMemoryStorageItem{key: key, data: value}) +func (m *memoryStorage) Store(ctx context.Context, key string, value []byte) error { + m.contents = append(m.contents, memoryStorageItem{key: key, data: value}) return nil } -func (m *testingMemoryStorage) Exists(ctx context.Context, key string) bool { +func (m *memoryStorage) Exists(ctx context.Context, key string) bool { return m.lookup(ctx, key) != nil } -func (m *testingMemoryStorage) List(ctx context.Context, path string, recursive bool) ([]string, error) { +func (m *memoryStorage) List(ctx context.Context, path string, recursive bool) ([]string, error) { if recursive { panic("unimplemented") } @@ -88,22 +88,22 @@ nextitem: } return result, nil } -func (m *testingMemoryStorage) Load(ctx context.Context, key string) ([]byte, error) { +func (m *memoryStorage) Load(ctx context.Context, key string) ([]byte, error) { if item := m.lookup(ctx, key); item != nil { return item.data, nil } return nil, fs.ErrNotExist } -func (m *testingMemoryStorage) Stat(ctx context.Context, key string) (KeyInfo, error) { +func (m *memoryStorage) Stat(ctx context.Context, key string) (KeyInfo, error) { if item := m.lookup(ctx, key); item != nil { return KeyInfo{Key: key, Size: int64(len(item.data))}, nil } return KeyInfo{}, fs.ErrNotExist } -func (m *testingMemoryStorage) Lock(ctx context.Context, name string) error { panic("unimplemented") } -func (m *testingMemoryStorage) Unlock(ctx context.Context, name string) error { panic("unimplemented") } +func (m *memoryStorage) Lock(ctx context.Context, name string) error { panic("unimplemented") } +func (m *memoryStorage) Unlock(ctx context.Context, name string) error { panic("unimplemented") } -var _ Storage = (*testingMemoryStorage)(nil) +var _ Storage = (*memoryStorage)(nil) type recordingStorage struct { Storage @@ -293,7 +293,7 @@ func TestGetAccountAlreadyExistsSkipsBroken(t *testing.T) { am := &ACMEIssuer{CA: dummyCA, Logger: zap.NewNop(), mu: new(sync.Mutex)} testConfig := &Config{ Issuers: []Issuer{am}, - Storage: &testingMemoryStorage{}, + Storage: &memoryStorage{}, Logger: defaultTestLogger, certCache: new(Cache), } @@ -342,7 +342,7 @@ func TestGetAccountWithEmailAlreadyExists(t *testing.T) { am := &ACMEIssuer{CA: dummyCA, Logger: zap.NewNop(), mu: new(sync.Mutex)} testConfig := &Config{ Issuers: []Issuer{am}, - Storage: &recordingStorage{Storage: &testingMemoryStorage{}}, + Storage: &recordingStorage{Storage: &memoryStorage{}}, Logger: defaultTestLogger, certCache: new(Cache), } diff --git a/filestorage.go b/filestorage.go index d3df9cf7..2441498e 100644 --- a/filestorage.go +++ b/filestorage.go @@ -289,7 +289,7 @@ func fileLockIsStale(meta lockMeta) bool { // identified by filename. A successfully created // lockfile should be removed with removeLockfile. func createLockfile(filename string) error { - err := atomicallyCreateFile(filename, true) + err := atomicallyCreateFile(filename) if err != nil { return err } @@ -376,29 +376,26 @@ func updateLockfileFreshness(filename string) (bool, error) { // atomicallyCreateFile atomically creates the file // identified by filename if it doesn't already exist. -func atomicallyCreateFile(filename string, writeLockInfo bool) error { +func atomicallyCreateFile(filename string) error { // no need to check this error, we only really care about the file creation error _ = os.MkdirAll(filepath.Dir(filename), 0700) - f, err := os.OpenFile(filename, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0644) + fp, err := atomicfile.New(filename, 0o600) if err != nil { + // cancel the write if file creation fails and error out return err } - defer f.Close() - if writeLockInfo { - now := time.Now() - meta := lockMeta{ - Created: now, - Updated: now, - } - if err := json.NewEncoder(f).Encode(meta); err != nil { - return err - } - // see https://github.com/caddyserver/caddy/issues/3954 - if err := f.Sync(); err != nil { - return err - } + now := time.Now() + meta := lockMeta{ + Created: now, + Updated: now, } - return nil + if err := json.NewEncoder(fp).Encode(meta); err != nil { + // cancel the write if json encoding fails and error out + fp.Cancel() + return err + } + // close, thereby flushing the write + return fp.Close() } // homeDir returns the best guess of the current user's home diff --git a/memorystorage.go b/memorystorage.go deleted file mode 100644 index 0d44c8b7..00000000 --- a/memorystorage.go +++ /dev/null @@ -1,186 +0,0 @@ -// Copyright 2015 Matthew Holt -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package certmagic - -import ( - "context" - "os" - "path" - "strings" - "sync" - "time" - - "golang.org/x/sync/semaphore" -) - -type storageEntry struct { - i KeyInfo - d []byte -} - -// memoryStorage is a Storage implemention that exists only in memory -// it is intended for testing and one-time-deploys where no persistence is needed -type memoryStorage struct { - m map[string]storageEntry - mu sync.RWMutex - - kmu *keyMutex -} - -func NewMemoryStorage() Storage { - return &memoryStorage{ - m: map[string]storageEntry{}, - kmu: newKeyMutex(), - } -} - -// Exists returns true if key exists in s. -func (s *memoryStorage) Exists(ctx context.Context, key string) bool { - ans, err := s.List(ctx, key, true) - if err != nil { - return false - } - return len(ans) != 0 -} - -// Store saves value at key. -func (s *memoryStorage) Store(_ context.Context, key string, value []byte) error { - s.m[key] = storageEntry{ - i: KeyInfo{ - Key: key, - Modified: time.Now(), - Size: int64(len(value)), - IsTerminal: true, - }, - d: value, - } - return nil -} - -// Load retrieves the value at key. -func (s *memoryStorage) Load(_ context.Context, key string) ([]byte, error) { - val, ok := s.m[key] - if !ok { - return nil, os.ErrNotExist - } - return val.d, nil -} - -// Delete deletes the value at key. -func (s *memoryStorage) Delete(_ context.Context, key string) error { - delete(s.m, key) - return nil -} - -// List returns all keys that match prefix. -func (s *memoryStorage) List(ctx context.Context, prefix string, recursive bool) ([]string, error) { - var keyList []string - - keys := make([]string, 0, len(s.m)) - for k := range s.m { - if strings.HasPrefix(k, prefix) { - keys = append(keys, k) - } - } - // adapted from https://github.com/pberkel/caddy-storage-redis/blob/main/storage.go#L369 - // Iterate over each child key - for _, k := range keys { - // Directory keys will have a "/" suffix - trimmedKey := strings.TrimSuffix(k, "/") - // Reconstruct the full path of child key - fullPathKey := path.Join(prefix, trimmedKey) - // If current key is a directory - if recursive && k != trimmedKey { - // Recursively traverse all child directories - childKeys, err := s.List(ctx, fullPathKey, recursive) - if err != nil { - return keyList, err - } - keyList = append(keyList, childKeys...) - } else { - keyList = append(keyList, fullPathKey) - } - } - - return keys, nil -} - -// Stat returns information about key. -func (s *memoryStorage) Stat(_ context.Context, key string) (KeyInfo, error) { - val, ok := s.m[key] - if !ok { - return KeyInfo{}, os.ErrNotExist - } - return val.i, nil -} - -// Lock obtains a lock named by the given name. It blocks -// until the lock can be obtained or an error is returned. -func (s *memoryStorage) Lock(ctx context.Context, name string) error { - return s.kmu.LockKey(ctx, name) -} - -// Unlock releases the lock for name. -func (s *memoryStorage) Unlock(_ context.Context, name string) error { - return s.kmu.UnlockKey(name) -} - -func (s *memoryStorage) String() string { - return "memoryStorage" -} - -// Interface guard -var _ Storage = (*memoryStorage)(nil) - -type keyMutex struct { - m map[string]*semaphore.Weighted - mu sync.Mutex -} - -func newKeyMutex() *keyMutex { - return &keyMutex{ - m: map[string]*semaphore.Weighted{}, - } -} - -func (km *keyMutex) LockKey(ctx context.Context, id string) error { - select { - case <-ctx.Done(): - // as a special case, caddy allows for the cancelled context to be used for a trylock. - if km.mutex(id).TryAcquire(1) { - return nil - } - return ctx.Err() - default: - return km.mutex(id).Acquire(ctx, 1) - } -} - -// Releases the lock associated with the specified ID. -func (km *keyMutex) UnlockKey(id string) error { - km.mutex(id).Release(1) - return nil -} - -func (km *keyMutex) mutex(id string) *semaphore.Weighted { - km.mu.Lock() - defer km.mu.Unlock() - val, ok := km.m[id] - if !ok { - val = semaphore.NewWeighted(1) - km.m[id] = val - } - return val -} diff --git a/memorystorage_test.go b/memorystorage_test.go deleted file mode 100644 index a12e8f77..00000000 --- a/memorystorage_test.go +++ /dev/null @@ -1,72 +0,0 @@ -package certmagic_test - -import ( - "bytes" - "context" - "os" - "testing" - - "github.com/caddyserver/certmagic" - "github.com/caddyserver/certmagic/internal/testutil" -) - -func TestMemoryStorageStoreLoad(t *testing.T) { - ctx := context.Background() - tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*") - testutil.RequireNoError(t, err, "allocating tmp dir") - defer os.RemoveAll(tmpDir) - s := certmagic.NewMemoryStorage() - err = s.Store(ctx, "foo", []byte("bar")) - testutil.RequireNoError(t, err) - dat, err := s.Load(ctx, "foo") - testutil.RequireNoError(t, err) - testutil.RequireEqualValues(t, dat, []byte("bar")) -} - -func TestMemoryStorageStoreLoadRace(t *testing.T) { - ctx := context.Background() - tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*") - testutil.RequireNoError(t, err, "allocating tmp dir") - defer os.RemoveAll(tmpDir) - s := certmagic.NewMemoryStorage() - a := bytes.Repeat([]byte("a"), 4096*1024) - b := bytes.Repeat([]byte("b"), 4096*1024) - err = s.Store(ctx, "foo", a) - testutil.RequireNoError(t, err) - done := make(chan struct{}) - go func() { - err := s.Store(ctx, "foo", b) - testutil.RequireNoError(t, err) - close(done) - }() - dat, err := s.Load(ctx, "foo") - <-done - testutil.RequireNoError(t, err) - testutil.RequireEqualValues(t, 4096*1024, len(dat)) -} - -func TestMemoryStorageWriteLock(t *testing.T) { - ctx := context.Background() - tmpDir, err := os.MkdirTemp(os.TempDir(), "certmagic*") - testutil.RequireNoError(t, err, "allocating tmp dir") - defer os.RemoveAll(tmpDir) - s := certmagic.NewMemoryStorage() - // cctx is a cancelled ctx. so if we can't immediately get the lock, it will fail - cctx, cn := context.WithCancel(ctx) - cn() - // should success - err = s.Lock(cctx, "foo") - testutil.RequireNoError(t, err) - // should fail - err = s.Lock(cctx, "foo") - testutil.RequireError(t, err) - - err = s.Unlock(cctx, "foo") - testutil.RequireNoError(t, err) - // shouldn't fail - err = s.Lock(cctx, "foo") - testutil.RequireNoError(t, err) - - err = s.Unlock(cctx, "foo") - testutil.RequireNoError(t, err) -} From 2802ae39bf852fda55ae211e060293eadba446de Mon Sep 17 00:00:00 2001 From: a Date: Fri, 28 Mar 2025 15:16:28 -0500 Subject: [PATCH 3/6] writing --- filestorage.go | 77 +++++++++++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 32 deletions(-) diff --git a/filestorage.go b/filestorage.go index 2441498e..45e30ecb 100644 --- a/filestorage.go +++ b/filestorage.go @@ -332,51 +332,65 @@ func keepLockfileFresh(filename string) { // with the current timestamp. It returns true if the parent // loop can terminate (i.e. no more need to update the lock). func updateLockfileFreshness(filename string) (bool, error) { - f, err := os.OpenFile(filename, os.O_RDWR, 0644) - if os.IsNotExist(err) { + fileBytes, err := readFileLimit(filename, 2048) + if os.IsNotExist(err) { // this error is passed along through readFileLimit, so we can check for it here. return true, nil // lock released } if err != nil { return true, err } - defer f.Close() - - // read contents - metaBytes, err := io.ReadAll(io.LimitReader(f, 2048)) - if err != nil { - return true, err - } var meta lockMeta - if err := json.Unmarshal(metaBytes, &meta); err != nil { + if err := json.Unmarshal(fileBytes, &meta); err != nil { // see issue #232: this can error if the file is empty, // which happens sometimes when the disk is REALLY slow return true, err } - // truncate file and reset I/O offset to beginning - if err := f.Truncate(0); err != nil { - return true, err - } - if _, err := f.Seek(0, io.SeekStart); err != nil { - return true, err - } - // write updated timestamp meta.Updated = time.Now() - if err = json.NewEncoder(f).Encode(meta); err != nil { + newBytes, err := json.Marshal(meta) + if err != nil { return false, err } + if err = atomicallyWriteFile(filename, newBytes); err != nil { + // if we fail to write the file, we should probably exit, so return true here? + return true, err + } + return false, nil +} - // sync to device; we suspect that sometimes file systems - // (particularly AWS EFS) don't do this on their own, - // leaving the file empty when we close it; see - // https://github.com/caddyserver/caddy/issues/3954 - return false, f.Sync() +func readFileLimit(filename string, limit int) ([]byte, error) { + // open opens a file in RDONLY with mode 0. this is safe + fp, err := os.Open(filename) + if err != nil { + return nil, err + } + defer fp.Close() + limitReader := io.LimitReader(fp, int64(limit)) + readBytes, err := io.ReadAll(limitReader) + if err != nil { + return nil, err + } + return readBytes, nil } -// atomicallyCreateFile atomically creates the file +// atomicallyCreateFile atomically creates the lock file // identified by filename if it doesn't already exist. func atomicallyCreateFile(filename string) error { + now := time.Now() + meta := lockMeta{ + Created: now, + Updated: now, + } + metaBytes, err := json.Marshal(meta) + if err != nil { + return err + } + return atomicallyWriteFile(filename, metaBytes) +} + +// creates a file at filename with data in bytes atomically +func atomicallyWriteFile(filename string, data []byte) error { // no need to check this error, we only really care about the file creation error _ = os.MkdirAll(filepath.Dir(filename), 0700) fp, err := atomicfile.New(filename, 0o600) @@ -384,13 +398,12 @@ func atomicallyCreateFile(filename string) error { // cancel the write if file creation fails and error out return err } - now := time.Now() - meta := lockMeta{ - Created: now, - Updated: now, - } - if err := json.NewEncoder(fp).Encode(meta); err != nil { - // cancel the write if json encoding fails and error out + n, err := fp.Write(data) + if err != nil || n != len(data) { + if n != len(data) && err == nil { + err = fmt.Errorf("short write (%d of %d bytes written)", n, len(data)) + } + // cancel the write fp.Cancel() return err } From f316a2f9a669646b3af99702d1a20d38fc439882 Mon Sep 17 00:00:00 2001 From: a Date: Fri, 28 Mar 2025 15:19:29 -0500 Subject: [PATCH 4/6] rename function --- filestorage.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/filestorage.go b/filestorage.go index 45e30ecb..6681e3a6 100644 --- a/filestorage.go +++ b/filestorage.go @@ -289,7 +289,7 @@ func fileLockIsStale(meta lockMeta) bool { // identified by filename. A successfully created // lockfile should be removed with removeLockfile. func createLockfile(filename string) error { - err := atomicallyCreateFile(filename) + err := atomicallyCreateLockfile(filename) if err != nil { return err } @@ -374,9 +374,9 @@ func readFileLimit(filename string, limit int) ([]byte, error) { return readBytes, nil } -// atomicallyCreateFile atomically creates the lock file +// atomicallyCreateLockfile atomically creates the lock file // identified by filename if it doesn't already exist. -func atomicallyCreateFile(filename string) error { +func atomicallyCreateLockfile(filename string) error { now := time.Now() meta := lockMeta{ Created: now, From 848b135cafdfde18b7e0ceb705ad4fabd3b5940d Mon Sep 17 00:00:00 2001 From: a Date: Fri, 28 Mar 2025 23:46:58 -0500 Subject: [PATCH 5/6] add stat to check existence --- filestorage.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/filestorage.go b/filestorage.go index 6681e3a6..ae82de61 100644 --- a/filestorage.go +++ b/filestorage.go @@ -393,6 +393,11 @@ func atomicallyCreateLockfile(filename string) error { func atomicallyWriteFile(filename string, data []byte) error { // no need to check this error, we only really care about the file creation error _ = os.MkdirAll(filepath.Dir(filename), 0700) + // check if the file exists + _, err := os.Stat(filename) + if err == nil { + return os.ErrExist + } fp, err := atomicfile.New(filename, 0o600) if err != nil { // cancel the write if file creation fails and error out From 2d2532392b11dcdcea8b732d5647e191e49fafec Mon Sep 17 00:00:00 2001 From: a Date: Fri, 28 Mar 2025 23:49:18 -0500 Subject: [PATCH 6/6] add some race checks --- filestorage.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/filestorage.go b/filestorage.go index ae82de61..ec0f880a 100644 --- a/filestorage.go +++ b/filestorage.go @@ -393,7 +393,8 @@ func atomicallyCreateLockfile(filename string) error { func atomicallyWriteFile(filename string, data []byte) error { // no need to check this error, we only really care about the file creation error _ = os.MkdirAll(filepath.Dir(filename), 0700) - // check if the file exists + // check if the file exists no matter what you do, this is sort of racy. either you read the empty file, or you accidentaly open multiple files. + // using native os level locks would be better here. _, err := os.Stat(filename) if err == nil { return os.ErrExist @@ -412,6 +413,13 @@ func atomicallyWriteFile(filename string, data []byte) error { fp.Cancel() return err } + // do another check, as a race check. this still isnt perfect. + _, err = os.Stat(filename) + if err == nil { + // cancel the write + fp.Cancel() + return os.ErrExist + } // close, thereby flushing the write return fp.Close() }