From 2ad040db3bcc48b94b20e19e21c537c2f375a6f7 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Mon, 6 Nov 2023 11:45:10 -0800 Subject: [PATCH] ipn/store: improve FileStore.WriteState atomicity If an error occurs with FileStore.WriteState, it should not record the provided value as this results in an inconsistency between what is cached in memory and what is stored on disk. Also, update the documentation of StateStore.ReadState to indicate that the returned value should be treated as immutable. This property is assumed by the fact that FileStore.ReadState returns the same slice of bytes for repeated calls to the same key. Updates #cleanup Signed-off-by: Joe Tsai --- ipn/store.go | 5 +++-- ipn/store/stores.go | 14 ++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/ipn/store.go b/ipn/store.go index 3bef012ba..a771784ad 100644 --- a/ipn/store.go +++ b/ipn/store.go @@ -68,8 +68,9 @@ func CurrentProfileKey(userID string) StateKey { // StateStore persists state, and produces it back on request. type StateStore interface { - // ReadState returns the bytes associated with ID. Returns (nil, - // ErrStateNotExist) if the ID doesn't have associated state. + // ReadState returns the bytes associated with ID. + // It returns (nil, ErrStateNotExist) if the ID doesn't have associated state. + // The returned value must not be mutated. ReadState(id StateKey) ([]byte, error) // WriteState saves bs as the state associated with ID. // diff --git a/ipn/store/stores.go b/ipn/store/stores.go index 8bf3a24b0..9cef1eaf6 100644 --- a/ipn/store/stores.go +++ b/ipn/store/stores.go @@ -173,16 +173,22 @@ func (s *FileStore) ReadState(id ipn.StateKey) ([]byte, error) { } // WriteState implements the StateStore interface. -func (s *FileStore) WriteState(id ipn.StateKey, bs []byte) error { +func (s *FileStore) WriteState(id ipn.StateKey, bs []byte) (err error) { s.mu.Lock() defer s.mu.Unlock() - if bytes.Equal(s.cache[id], bs) { + bs0 := s.cache[id] + if bytes.Equal(bs0, bs) { return nil } + defer func() { + if err != nil { + s.cache[id] = bs0 + } + }() s.cache[id] = bytes.Clone(bs) - bs, err := json.MarshalIndent(s.cache, "", " ") + b, err := json.MarshalIndent(s.cache, "", " ") if err != nil { return err } - return atomicfile.WriteFile(s.path, bs, 0600) + return atomicfile.WriteFile(s.path, b, 0600) }