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

Potential crash when sorting files in a directory #1200

Closed
joelim-work opened this issue Apr 5, 2023 · 4 comments · Fixed by #1204
Closed

Potential crash when sorting files in a directory #1200

joelim-work opened this issue Apr 5, 2023 · 4 comments · Fixed by #1204

Comments

@joelim-work
Copy link
Collaborator

When a directory is loaded but the settings for displaying files are modified, the following code is run:

lf/nav.go

Lines 473 to 477 in 83af3e1

go func() {
dir.sort()
dir.loading = false
nav.dirChan <- dir
}()

This code looks very suspicious to me because it accesses and sorts dir.files in a different goroutine, when normally this is manipulated by the main thread. I suspect this is what causes lf to crash as reported in #502, and I have also experienced crashing when I was playing around with the dironly option.

I was thinking that this could be solved by simply creating a copy for the goroutine like below:

--- a/nav.go
+++ b/nav.go
@@ -470,10 +470,11 @@ func (nav *nav) checkDir(dir *dir) {
                dir.ignorecase != gOpts.ignorecase ||
                dir.ignoredia != gOpts.ignoredia:
                dir.loading = true
+               sd := *dir
                go func() {
-                       dir.sort()
-                       dir.loading = false
-                       nav.dirChan <- dir
+                       sd.sort()
+                       sd.loading = false
+                       nav.dirChan <- &sd
                }()
        }
 }

But since this is hard to prove and I'm not 100% certain, I'm raising this as an issue first.

@joelim-work
Copy link
Collaborator Author

I managed to produce the crash again on version r28, below is the stack trace:

panic: runtime error: index out of range [61] with length 60

goroutine 82 [running]:
main.(*dir).sort.func11(0x3b?, 0x3a?)
	github.com/gokcehan/lf/nav.go:283 +0x12f
sort.insertionSort_func({0xc000718f88?, 0xc0008068a0?}, 0x3c, 0x50)
	sort/zsortfunc.go:12 +0xb1
sort.stable_func({0xc000718f88?, 0xc0008068a0?}, 0x53)
	sort/zsortfunc.go:339 +0x4a
sort.SliceStable({0x55a91a5f98a0, 0xc000012600}, 0x53?)
	sort/slice.go:35 +0x85
main.(*dir).sort(0xc0000c65b0)
	github.com/gokcehan/lf/nav.go:282 +0x47c
main.(*nav).checkDir.func2()
	github.com/gokcehan/lf/nav.go:469 +0x2b
created by main.(*nav).checkDir
	github.com/gokcehan/lf/nav.go:468 +0x1ef

BTW I have found that it's convenient to redirect stderr to a file so that the stack trace can be viewed easily:

lf 2> ~/lf.log

@joelim-work
Copy link
Collaborator Author

I believe this was detected and fixed in #759, but the commit was later reverted in 8954e9f. I'm not sure why but possibly the commit contained a bunch of other changes which might have caused other issues.

@gokcehan
Copy link
Owner

gokcehan commented Apr 8, 2023

@joelim-work Your proposed change sounds reasonable to me. We can change it and try it for a while if you want. Is there a way to reproduce the crash in a consistent manner?

@joelim-work
Copy link
Collaborator Author

@gokcehan I first noticed this crashing behaviour when I was using the dironly option to improve upon the move in parent directory operation (I have now added this to the wiki):

map J :updir; set dironly true; down; set dironly false; open
map K :updir; set dironly true; up; set dironly false; open

I can consistently reproduce this crash on my machine by using it to select a very large directory, but because the crashing behaviour depends on goroutine scheduling and the filesystem, I don't know how easy it is for others to reproduce. I have created a pull request #1204, which fixes the issue for me, and hopefully others as well.

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 a pull request may close this issue.

2 participants