From 04e36fdb52e0446b9166dc91e2cc094693f90cf0 Mon Sep 17 00:00:00 2001 From: Mathieu Agar Date: Tue, 6 May 2025 09:52:52 +0200 Subject: [PATCH 1/3] fix: Race condition in `nativeGitClient.LsFiles` Changing the current working directory is not safe for concurrent use. To avoid this, I am adding the repository root as a prefix to the pattern. To make sure the results are relevant I also needed to evaluate symlinks in the repository root path, in case part of it is a symlink (which is the case in for temporary directories on Mac OS). Fixes #21754 Signed-off-by: Mathieu Agar --- util/git/client.go | 17 +++++++++--- util/git/client_test.go | 60 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/util/git/client.go b/util/git/client.go index 2e31d34eb3c2f..d429556134bb6 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -422,11 +422,14 @@ func (m *nativeGitClient) Fetch(revision string) error { func (m *nativeGitClient) LsFiles(path string, enableNewGitFileGlobbing bool) ([]string, error) { if enableNewGitFileGlobbing { // This is the new way with safer globbing - err := os.Chdir(m.root) + + // evaluating the root path for symlinks + realRoot, err := filepath.EvalSymlinks(m.root) if err != nil { return nil, err } - allFiles, err := doublestar.FilepathGlob(path) + // searching for the pattern inside the root path + allFiles, err := doublestar.FilepathGlob(filepath.Join(realRoot, path)) if err != nil { return nil, err } @@ -440,8 +443,14 @@ func (m *nativeGitClient) LsFiles(path string, enableNewGitFileGlobbing bool) ([ if err != nil { return nil, err } - if strings.HasPrefix(absPath, m.root) { - files = append(files, file) + + if strings.HasPrefix(absPath, realRoot) { + // removing the repository root prefix from the file path + relativeFile, err := filepath.Rel(realRoot, file) + if err != nil { + return nil, err + } + files = append(files, relativeFile) } else { log.Warnf("Absolute path for %s is outside of repository, removing it", file) } diff --git a/util/git/client_test.go b/util/git/client_test.go index 951a67a62afe6..ac827c853b96a 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -9,6 +9,7 @@ import ( "path" "path/filepath" "strings" + "sync" "testing" "time" @@ -993,6 +994,65 @@ func Test_nativeGitClient_runCredentialedCmd(t *testing.T) { } } +func Test_LsFiles_RaceCondition(t *testing.T) { + // Create two temporary directories and initialize them as git repositories + tempDir1 := t.TempDir() + tempDir2 := t.TempDir() + + client1, err := NewClient("file://"+tempDir1, NopCreds{}, true, false, "", "") + require.NoError(t, err) + client2, err := NewClient("file://"+tempDir2, NopCreds{}, true, false, "", "") + require.NoError(t, err) + + err = client1.Init() + require.NoError(t, err) + err = client2.Init() + require.NoError(t, err) + + // Add different files to each repository + file1 := filepath.Join(client1.Root(), "file1.txt") + err = os.WriteFile(file1, []byte("content1"), 0644) + require.NoError(t, err) + err = runCmd(client1.Root(), "git", "add", "file1.txt") + require.NoError(t, err) + err = runCmd(client1.Root(), "git", "commit", "-m", "Add file1") + require.NoError(t, err) + + file2 := filepath.Join(client2.Root(), "file2.txt") + err = os.WriteFile(file2, []byte("content2"), 0644) + require.NoError(t, err) + err = runCmd(client2.Root(), "git", "add", "file2.txt") + require.NoError(t, err) + err = runCmd(client2.Root(), "git", "commit", "-m", "Add file2") + require.NoError(t, err) + + // Assert that LsFiles returns the correct files when called sequentially + files1, err := client1.LsFiles("*", true) + require.NoError(t, err) + require.Contains(t, files1, "file1.txt") + + files2, err := client2.LsFiles("*", true) + require.NoError(t, err) + require.Contains(t, files2, "file2.txt") + + // Define a function to call LsFiles multiple times in parallel + var wg sync.WaitGroup + callLsFiles := func(client Client, expectedFile string) { + defer wg.Done() + for i := 0; i < 100; i++ { + files, err := client.LsFiles("*", true) + require.NoError(t, err) + require.Contains(t, files, expectedFile) + } + } + + // Call LsFiles in parallel for both clients + wg.Add(2) + go callLsFiles(client1, "file1.txt") + go callLsFiles(client2, "file2.txt") + wg.Wait() +} + type mockCreds struct { environ []string environErr bool From 0a52d43969a84fd2e37d26fcebeef7ba4e335cf1 Mon Sep 17 00:00:00 2001 From: Mathieu Agar Date: Tue, 6 May 2025 10:25:03 +0200 Subject: [PATCH 2/3] fix: Use prefix for octal integer literal gofumpt requires a 0o prefix instead of the standard 0 for octal literals Signed-off-by: Mathieu Agar --- util/git/client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util/git/client_test.go b/util/git/client_test.go index ac827c853b96a..cef15c08c58f6 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -1011,7 +1011,7 @@ func Test_LsFiles_RaceCondition(t *testing.T) { // Add different files to each repository file1 := filepath.Join(client1.Root(), "file1.txt") - err = os.WriteFile(file1, []byte("content1"), 0644) + err = os.WriteFile(file1, []byte("content1"), 0o644) require.NoError(t, err) err = runCmd(client1.Root(), "git", "add", "file1.txt") require.NoError(t, err) @@ -1019,7 +1019,7 @@ func Test_LsFiles_RaceCondition(t *testing.T) { require.NoError(t, err) file2 := filepath.Join(client2.Root(), "file2.txt") - err = os.WriteFile(file2, []byte("content2"), 0644) + err = os.WriteFile(file2, []byte("content2"), 0o644) require.NoError(t, err) err = runCmd(client2.Root(), "git", "add", "file2.txt") require.NoError(t, err) From f6e5028882f3637471f8e2212258b2e91eb51ebf Mon Sep 17 00:00:00 2001 From: Mathieu Agar Date: Wed, 7 May 2025 16:51:57 +0200 Subject: [PATCH 3/3] change wording of warning Using ignore instead of remove makes it more explicit Co-authored-by: rumstead <37445536+rumstead@users.noreply.github.com> Signed-off-by: Mathieu Agar --- util/git/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/git/client.go b/util/git/client.go index d429556134bb6..631156e836968 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -452,7 +452,7 @@ func (m *nativeGitClient) LsFiles(path string, enableNewGitFileGlobbing bool) ([ } files = append(files, relativeFile) } else { - log.Warnf("Absolute path for %s is outside of repository, removing it", file) + log.Warnf("Absolute path for %s is outside of repository, ignoring it", file) } } return files, nil