From 29bc145fa9ecef140660dd6d6bf3071b0f3908aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Fri, 3 Nov 2023 17:27:47 +0200 Subject: [PATCH] extkingpin: fix Content/Rewrite race (#6870) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Got this during `TestLimiter_StartConfigReloader`: ``` Read at 0x00c0005123f0 by goroutine 8711: github.com/thanos-io/thanos/pkg/extkingpin.(*staticPathContent).Content() /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:90 +0x28 github.com/thanos-io/thanos/pkg/receive.ParseLimitConfigContent() /home/giedrius/dev/thanos/pkg/receive/limiter.go:203 +0x3e github.com/thanos-io/thanos/pkg/receive.(*Limiter).loadConfig() /home/giedrius/dev/thanos/pkg/receive/limiter.go:143 +0x65 github.com/thanos-io/thanos/pkg/receive.(*Limiter).StartConfigReloader.func1() /home/giedrius/dev/thanos/pkg/receive/limiter.go:119 +0x207 github.com/thanos-io/thanos/pkg/extkingpin.(*pollingEngine).start.func1() /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:64 +0x6b8 github.com/thanos-io/thanos/pkg/extkingpin.(*pollingEngine).start.func2() /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:74 +0x56 Previous write at 0x00c0005123f0 by goroutine 8710: github.com/thanos-io/thanos/pkg/extkingpin.(*staticPathContent).Rewrite() /home/giedrius/dev/thanos/pkg/extkingpin/path_content_reloader.go:111 +0x529 github.com/thanos-io/thanos/pkg/receive.TestLimiter_StartConfigReloader() /home/giedrius/dev/thanos/pkg/receive/limiter_test.go:44 +0x5aa testing.tRunner() /usr/lib/go-1.21/src/testing/testing.go:1595 +0x238 testing.(*T).Run.func1() /usr/lib/go-1.21/src/testing/testing.go:1648 +0x44 ``` Fix it by protecting `t.content` with a mutex and by copying the content's slice so that the caller wouldn't have access to the original slice. Signed-off-by: Giedrius Statkevičius Signed-off-by: Sebastian Rabenhorst --- pkg/extkingpin/path_content_reloader.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/pkg/extkingpin/path_content_reloader.go b/pkg/extkingpin/path_content_reloader.go index e0d8fdf74ba..e96b0ddb34a 100644 --- a/pkg/extkingpin/path_content_reloader.go +++ b/pkg/extkingpin/path_content_reloader.go @@ -8,6 +8,7 @@ import ( "crypto/sha256" "os" "path/filepath" + "sync" "time" "github.com/go-kit/log" @@ -79,15 +80,22 @@ func (p *pollingEngine) start(ctx context.Context) error { } type staticPathContent struct { - content []byte - path string + contentMtx sync.Mutex + content []byte + path string } var _ fileContent = (*staticPathContent)(nil) // Content returns the cached content. func (t *staticPathContent) Content() ([]byte, error) { - return t.content, nil + t.contentMtx.Lock() + defer t.contentMtx.Unlock() + + c := make([]byte, 0, len(t.content)) + c = append(c, t.content...) + + return c, nil } // Path returns the path to the file that contains the content. @@ -102,12 +110,15 @@ func NewStaticPathContent(fromPath string) (*staticPathContent, error) { if err != nil { return nil, errors.Wrapf(err, "could not load test content: %s", fromPath) } - return &staticPathContent{content, fromPath}, nil + return &staticPathContent{content: content, path: fromPath}, nil } // Rewrite rewrites the file backing this staticPathContent and swaps the local content cache. The file writing // is needed to trigger the file system monitor. func (t *staticPathContent) Rewrite(newContent []byte) error { + t.contentMtx.Lock() + defer t.contentMtx.Unlock() + t.content = newContent // Write the file to ensure possible file watcher reloaders get triggered. return os.WriteFile(t.path, newContent, 0666)