From ffd1152cdc805d3fae246f8846c7d810f6e3184d Mon Sep 17 00:00:00 2001 From: per1234 Date: Tue, 25 May 2021 22:23:53 -0700 Subject: [PATCH 1/3] Add test for project discovery handling of symlink loops --- internal/project/project_test.go | 46 ++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/internal/project/project_test.go b/internal/project/project_test.go index 825e7ade..9a72a22e 100644 --- a/internal/project/project_test.go +++ b/internal/project/project_test.go @@ -20,12 +20,14 @@ import ( "os" "reflect" "testing" + "time" "github.com/arduino/arduino-lint/internal/configuration" "github.com/arduino/arduino-lint/internal/project/projecttype" "github.com/arduino/arduino-lint/internal/util/test" "github.com/arduino/go-paths-helper" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) var testDataPath *paths.Path @@ -38,6 +40,50 @@ func init() { testDataPath = paths.New(workingDirectory, "testdata") } +func TestSymlinkLoop(t *testing.T) { + // Set up directory structure of test library. + libraryPath, err := paths.TempDir().MkTempDir("TestSymlinkLoop") + defer libraryPath.RemoveAll() // Clean up after the test. + require.Nil(t, err) + err = libraryPath.Join("TestSymlinkLoop.h").WriteFile([]byte{}) + require.Nil(t, err) + examplesPath := libraryPath.Join("examples") + err = examplesPath.Mkdir() + require.Nil(t, err) + + // It's probably most friendly for contributors using Windows to create the symlinks needed for the test on demand. + err = os.Symlink(examplesPath.Join("..").String(), examplesPath.Join("UpGoer1").String()) + require.Nil(t, err, "This test must be run as administrator on Windows to have symlink creation privilege.") + // It's necessary to have multiple symlinks to a parent directory to create the loop. + err = os.Symlink(examplesPath.Join("..").String(), examplesPath.Join("UpGoer2").String()) + require.Nil(t, err) + + configuration.Initialize(test.ConfigurationFlags(), []string{libraryPath.String()}) + + // The failure condition is FindProjects() never returning, testing for which requires setting up a timeout. + done := make(chan bool) + go func() { + _, err = FindProjects() + done <- true + }() + + assert.Eventually( + t, + func() bool { + select { + case <-done: + return true + default: + return false + } + }, + 20*time.Second, + 10*time.Millisecond, + "Infinite symlink loop during project discovery", + ) + require.Nil(t, err) +} + func TestFindProjects(t *testing.T) { sketchPath := testDataPath.Join("Sketch") libraryPath := testDataPath.Join("Library") From f96e65bb065b1322b2e8a464f331d27089870f58 Mon Sep 17 00:00:00 2001 From: per1234 Date: Wed, 26 May 2021 08:06:16 -0700 Subject: [PATCH 2/3] Avoid hang during discovery in projects with symlink loops Specific combinations of symlinks can cause infinite recursion loops during project discovery. This is avoided by limiting the depth of symlink follows to a reasonable number. --- internal/project/project.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/internal/project/project.go b/internal/project/project.go index 2d88a52d..c7da7362 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -18,6 +18,7 @@ package project import ( "fmt" + "os" "github.com/arduino/arduino-lint/internal/configuration" "github.com/arduino/arduino-lint/internal/project/library" @@ -87,7 +88,7 @@ func findProjects(targetPath *paths.Path) ([]Type, error) { } else { if configuration.SuperprojectTypeFilter() == projecttype.All || configuration.Recursive() { // Project discovery and/or type detection is required. - foundParentProjects = findProjectsUnderPath(targetPath, configuration.SuperprojectTypeFilter(), configuration.Recursive()) + foundParentProjects = findProjectsUnderPath(targetPath, configuration.SuperprojectTypeFilter(), configuration.Recursive(), 0) } else { // Project was explicitly defined by user. foundParentProjects = append(foundParentProjects, @@ -115,7 +116,7 @@ func findProjects(targetPath *paths.Path) ([]Type, error) { } // findProjectsUnderPath finds projects of the given type under the given path. It returns a slice containing the definitions of all found projects. -func findProjectsUnderPath(targetPath *paths.Path, projectTypeFilter projecttype.Type, recursive bool) []Type { +func findProjectsUnderPath(targetPath *paths.Path, projectTypeFilter projecttype.Type, recursive bool, symlinkDepth int) []Type { var foundProjects []Type isProject, foundProjectType := isProject(targetPath, projectTypeFilter) @@ -133,12 +134,24 @@ func findProjectsUnderPath(targetPath *paths.Path, projectTypeFilter projecttype return foundProjects } - if recursive { + if recursive && symlinkDepth < 10 { // targetPath was not a project, so search the subfolders. directoryListing, _ := targetPath.ReadDir() directoryListing.FilterDirs() for _, potentialProjectDirectory := range directoryListing { - foundProjects = append(foundProjects, findProjectsUnderPath(potentialProjectDirectory, projectTypeFilter, recursive)...) + // It is possible for a combination of symlinks to parent paths to cause project discovery to get stuck in + // an endless loop of recursion. This is avoided by keeping count of the depth of symlinks and discontinuing + // recursion when it exceeds reason. + pathStat, err := os.Lstat(potentialProjectDirectory.String()) + if err != nil { + panic(err) + } + depthDelta := 0 + if pathStat.Mode()&os.ModeSymlink != 0 { + depthDelta = 1 + } + + foundProjects = append(foundProjects, findProjectsUnderPath(potentialProjectDirectory, projectTypeFilter, recursive, symlinkDepth+depthDelta)...) } } @@ -184,7 +197,7 @@ func findSubprojects(superproject Type, apexSuperprojectType projecttype.Type) [ directoryListing.FilterDirs() for _, subprojectPath := range directoryListing { - immediateSubprojects = append(immediateSubprojects, findProjectsUnderPath(subprojectPath, subProjectType, searchPathsRecursively)...) + immediateSubprojects = append(immediateSubprojects, findProjectsUnderPath(subprojectPath, subProjectType, searchPathsRecursively, 0)...) } } } From 17c7b7c6f6b955b92649c9b890ec863eb2e1634d Mon Sep 17 00:00:00 2001 From: per1234 Date: Wed, 26 May 2021 09:01:24 -0700 Subject: [PATCH 3/3] Panic when symlink loop encountered during project discovery Even though the previous commit caused the project discovery code to gracefully avoid perpetual recursion when specific configurations of symlinks are present in the linting target path, it turned out that the project data phase that comes after is also not able to handle this correctly, and fixing that is more difficult since the incompatible code is in Arduino CLI. So for now the provisional fix is to panic during the project discovery phase, which will at least avoid the far worse behavior of a permanent hang. --- internal/project/project.go | 5 ++++- internal/project/project_test.go | 24 +----------------------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/internal/project/project.go b/internal/project/project.go index c7da7362..b5728533 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -134,7 +134,10 @@ func findProjectsUnderPath(targetPath *paths.Path, projectTypeFilter projecttype return foundProjects } - if recursive && symlinkDepth < 10 { + if recursive { + if symlinkDepth > 10 { + panic(fmt.Sprintf("symlink depth exceeded maximum while finding projects under %s", targetPath)) + } // targetPath was not a project, so search the subfolders. directoryListing, _ := targetPath.ReadDir() directoryListing.FilterDirs() diff --git a/internal/project/project_test.go b/internal/project/project_test.go index 9a72a22e..25a1a31e 100644 --- a/internal/project/project_test.go +++ b/internal/project/project_test.go @@ -20,7 +20,6 @@ import ( "os" "reflect" "testing" - "time" "github.com/arduino/arduino-lint/internal/configuration" "github.com/arduino/arduino-lint/internal/project/projecttype" @@ -60,28 +59,7 @@ func TestSymlinkLoop(t *testing.T) { configuration.Initialize(test.ConfigurationFlags(), []string{libraryPath.String()}) - // The failure condition is FindProjects() never returning, testing for which requires setting up a timeout. - done := make(chan bool) - go func() { - _, err = FindProjects() - done <- true - }() - - assert.Eventually( - t, - func() bool { - select { - case <-done: - return true - default: - return false - } - }, - 20*time.Second, - 10*time.Millisecond, - "Infinite symlink loop during project discovery", - ) - require.Nil(t, err) + assert.Panics(t, func() { FindProjects() }, "Infinite symlink loop encountered during project discovery") } func TestFindProjects(t *testing.T) {