Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the infinite recursion with afero.Walk #409

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erstam
Copy link

@erstam erstam commented Nov 14, 2023

This PR fixes #185

Fix the infinite recursion by skipping the current directory in the directory names listing

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2023

CLA assistant check
All committers have signed the CLA.

@erstam
Copy link
Author

erstam commented Nov 15, 2023

@spf13 Please, who can review this PR ?

@bep
Copy link
Collaborator

bep commented Nov 28, 2023

@erstam this needs a simple test (that does infinite recursion without this patch).

@erstam
Copy link
Author

erstam commented Dec 4, 2023

@bep I worked more on this issue and noticed something interesting. In my code where I use afero, I am removing the root folder of the fs instance and this causes the infinite recursion when using the Walk method. Should removing the root be forbidden on all Fs types ? I see in MemMapFs there is a init sync.Once attribute which is used to initialize a "root path" only once. But what if we delete that root path ? It kinda breaks the Fs.

Btw, using Windows here, if ever it matters.

The below test function will reproduce the issue.

func TestWalkRemoveRoot(t *testing.T) {
	defer removeAllTestFiles(t)
	fs := Fss[0]
	rootPath := "."

	err := fs.RemoveAll(rootPath)
	if err != nil {
		t.Error(err)
	}

	testRegistry[fs] = append(testRegistry[fs], rootPath)
	setupTestFiles(t, fs, rootPath)

	walkFn := func(path string, info os.FileInfo, err error) error {
		fmt.Println(path, info.Name(), info.IsDir(), info.Size(), err)
		return err
	}

	err = Walk(fs, rootPath, walkFn)
	if err != nil {
		t.Error(err)
	}
}

Seeing this case implies my proposed fix is just a workaround. The real fix would be to better protect the root path from deletion.

@erstam
Copy link
Author

erstam commented Dec 12, 2023

@bep, @spf13 any thoughts on my previous comment ?

@erstam
Copy link
Author

erstam commented Feb 7, 2024

Will we see this merged at some point ?

Repository owner deleted a comment from rapkaan Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MemMapFs + afero.Walk -> infinite loop
3 participants