Bug: lf crashes when shell command is run in config file

[SPFabGerman]

When running shell commands in the config file, lf immediately crashes upon startup. To replicate I have created a simple lfrc, only containing one command:

$echo "Hello World"

(Using !echo "Hello World" also works, but %echo "Hello World" and &echo "Hello World" do not produce any issues.)

lf crashes immediately with the following output:

Hello World
panic: runtime error: index out of range [-1]

goroutine 1 [running]:
main.(*nav).currDir(...)
	/home/fabian/build/lf/nav.go:1983
main.(*app).runCmdSync(0xc0001680c0, 0xc0002c0420, 0x0)
	/home/fabian/build/lf/app.go:487 +0x22c
main.(*app).runShell(0xc0001680c0, {0xc00016ac90, 0x12}, {0x0, 0x0, 0x0}, {0xc0002f4b6c, 0x1})
	/home/fabian/build/lf/app.go:523 +0x5b2
main.(*execExpr).eval(0xc000594b80, 0xc0001680c0, {0x0, 0x0, 0x0})
	/home/fabian/build/lf/eval.go:2180 +0x18f
main.(*app).readFile(0xc0001680c0, {0xc00012c160, 0x1c})
	/home/fabian/build/lf/app.go:97 +0x19a
main.(*app).loop(0xc0001680c0)
	/home/fabian/build/lf/app.go:266 +0x1305
main.run()
	/home/fabian/build/lf/client.go:74 +0x3f7
main.main()
	/home/fabian/build/lf/main.go:357 +0x819

A quick git bisect showed that the bug only appears after the recent #1638 PR. (Tagging @valoq @joelim-work, since they both discussed this PR.)

I haven't looked into the issue much yet, but it appears as if the nav.dirs array is empty during the initialization, causing the currDir() function to panic. I think the issue is probably the change in the runCmdSync() function. A quick fix would be to simply check if nav.dirs contains elements before calling the currDir() function or to have the currDir() function return a null pointer and then check the result of the call in the runCmdSync(), before setting the parameter. But I'm not too familiar with the code anymore, so there may be better solutions, that I'm not aware of. (Like loading the current directory before parsing the configuration file.)

[joelim-work]

Thanks very much for the bug report. I did not consider the scenario where shell commands are scripted from the config file - these will be run before lf begins to load any directories.

Certain operations don't make sense to run before lf has initialized, for instance getting the current directory. So as a quick fix we could just check app.nav.init like this:

diff --git a/app.go b/app.go
index e58acbf..717660a 100644
--- a/app.go
+++ b/app.go
@@ -484,7 +484,9 @@ func (app *app) runCmdSync(cmd *exec.Cmd, pause_after bool) {
 	app.ui.loadFile(app, true)

 	//mark the current directory as updated for refresh
-	app.nav.currDir().updated = true
+	if app.nav.init {
+		app.nav.currDir().updated = true
+	}

 	app.nav.renew()
 }

But after thinking about the problem some more, I am starting to think that reloading the current directory like this isn't a good solution for a few reasons:

• Shell commands do not necessarily modify anything (e.g. ls), and we already have the period option for picking up changes (although that is limited to directories being updated, not individual files).

• A shell command might modify the parent directory instead, so reloading the current directory won't help. We could alternatively reload all the directories afterwards, but this would probably cause performance issues, and the user can anyway append reload at the end of shell commands, for example see Update file permissions when returning from shell #1213 (comment).

• For copying/moving files, reloading the current directory actually doesn't work if you copy a large file and then cd to a different directory before the file finishes copying (something I only discovered today). This can be fixed with a patch like below, but it's already getting a bit messy (also not threadsafe):

  diff --git a/nav.go b/nav.go
  index d8830c1..32fa3fe 100644
  --- a/nav.go
  +++ b/nav.go
  @@ -1374,7 +1374,11 @@ loop:
   		app.ui.exprChan <- &callExpr{"echo", []string{"\033[0;32mCopied successfully\033[0m"}, 1}
   	}
   	//mark the current directory as updated for refresh
  -	nav.currDir().updated = true
  +	if gOpts.dircache {
  +		if d, ok := nav.dirCache[dstDir]; ok {
  +			d.updated = true
  +		}
  +	}
   }
   
   func (nav *nav) moveAsync(app *app, srcs []string, dstDir string) {
  @@ -1489,7 +1493,11 @@ func (nav *nav) moveAsync(app *app, srcs []string, dstDir string) {
   		app.ui.exprChan <- &callExpr{"echo", []string{"\033[0;32mMoved successfully\033[0m"}, 1}
   	}
   	//mark the current directory as updated for refresh
  -	nav.currDir().updated = true
  +	if gOpts.dircache {
  +		if d, ok := nav.dirCache[dstDir]; ok {
  +			d.updated = true
  +		}
  +	}
   }
   
   func (nav *nav) paste(app *app) error {

This kind of problem is not trivial to fix, and I think it requires further discussion. Instead of the above solution, I prefer to have either:

• A way to selectively load individual directories/files
• app.nav.renew check files in addition to directories

For the time being, since there is an upcoming release, I will revert #1638 as crashes can't be tolerated. Thanks once again for raising this.

[joelim-work]

This is fixed now since I reverted the commit, cc @gokcehan

[valoq]

@joelim-work
For the "return from shell" use case, how about going back to the original idea to only trigger an update when the interactive shell was used by adding app.nav.currDir().updated = true here: https://github.com/valoq/lf/blob/422e8236bbabde9c85ff5198b861e81d31815988/app.go#L527

The dircache condition you suggested looks like a good solution to me and since the updated variable does not itself trigger checkDir, I am not sure where you see the remaining threading issue. It may set the wrong directory to be updated in some cases but this should still be safe, right? Still a bug for those edge cases but in most scenarios it solves the issue.

To fully solve all issues with file changes, I think there isn't really a good alternative to using https://github.com/fsnotify/fsnotify, especially not without performance costs. That may be something to be revisited by @gokcehan eventually.

If at all possible I would like to get a fixed version of #1638 into the next release even if there may be a better solution in the future.

[gokcehan]

@valoq I would be happy to see a fixed version of the patch. Note, I was thinking of releasing r32 tomorrow or the day after that, so the patch might only be included in r33 onwards if that's ok. Or alternatively, we can delay the release further if people think it is worth including this patch in the release.

Regarding fsnotify, I'm ready to revisit the idea if someone comes up with a patch. However, I think the problem is that it may not be trivial to come up with such a patch, and at this point we are not sure if it is going to solve all our issues. There are simply too many different cases that users report as issues (e.g. permission changes, various filesystem mounts, directory counts, previews), so I'm not sure fsnotify will be able to handle all cases. In that case, we would need to maintain our use of fsnotify in addition to all the existing issues that we are getting reported.

@joelim-work I forgot if you shared your opinion somewhere before, but what do you think of fsnotify in general?

[joelim-work]

@valoq @gokcehan As a quick fix, I think it is fine to the merge the initial patch for #1638, namely this commit, as it doesn't have anything to do with the crash when running shell commands upon startup.

But I still see it as only a half-solution (but kind of good enough) with limitations, and I wouldn't surprised if there are further issues, in addition to what I mentioned above in #1659 (comment). For instance I discovered that this won't work when you copy from one instance to another - only the active instance will be updated, which makes sense as the updated flag isn't synchronized across the different instances.

On the topic of fsnotify, I think it is a good idea for file managers in general to automatically detect changes in files. If the UI isn't updated accordingly, it would be annoying for users, and potentially even have dangerous consequences. Actually I have worked on this a bit myself when I was reviewing #1548, but I agree that such a feature is far from trivial to implement, and like Sixel support, will require support/maintenance effort in future.

This is the work I have so far for fsnotify, which is now submitted as a draft PR: #1667. I have only done some testing, hopefully others can test it too and provide feedback or suggestions. I think it is better to leave this out for a while, before thinking about merging it.