[preview-tui] updated tree breaks preview file/dir list

[duffydack]

Environment details (Put x in the checkbox along with the information)

• Operating System: Arch Linux
• Desktop Environment: Sway
• Terminal Emulator: foot
• Shell: zsh
• Custom desktop opener (if applicable):
• Program options used:
• Configuration options set:
• Plugins are installed
• Issue exists on nnn master

Arch Linux with tree 2.0.1-2, using preview-tui script, now file/dir listings don't display correctly anymore. See [1]
Running the same tree command that preview-tui uses, in a shell, it still works. See [2]
Downgrading to 1.8.0-2 fixes, as expected.

[1] https://0x0.st/ooQa.png
[2] https://0x0.st/ooQB.png

[luukvbaal]

Seems like tree's -J flag is active in your preview-tui.

It's not there on master and I have no idea how it could be caused by updating, -J flag is the same for 1.8 -> 2.0.

The issue is also not present for me on 2.0.1-2, also on Arch. Please debug yourself.

[duffydack]

Didn't even think to try another terminal. Tried kitty, it's fine, so it's foot that's the issue. /sigh
I'll look into it and report if I find anything, in case anyone searches for same issue.

[badxnach]

This issue seems not to be limited to preview-tui.
I see the same as in duffydack's Picture 1 when I run "tree" within a shell opened with nnn by pressing ! or ^]

tree works fine in both xterm and urxvt without nnn

[dnkl]

tree automatically enables JSON output if the file descriptor STDDATA_FILENO exists.

This is, as far as I know, not a standardized file descriptor (like stdin/stdout/stderr), but rather something made up by tree. It is defined (in the tree source code) to 3. treeonly checks for this file descriptor, it does not verify its type or perform any other sanity checks.

In my case, tree-under-zsh-under-nnn has an inotify descriptor opened at FD 3. As far as I can tell, it's nnn that has opened it, and it is then being inherited from nnn to zsh, and finally to tree.

The following wrapper script shows that FD 3 is the cause of all this (it closes FD 3 and then exec's tree):

#!/usr/bin/bash
exec 3>&-
exec tree

[dnkl]

nnn/src/nnn.c

Line 8635 in 23b54ba

inotify_fd = inotify_init1(IN_NONBLOCK);

Should probably set IN_CLOEXEC.

[N-R-K]

tree automatically enables JSON output if the file descriptor STDDATA_FILENO exists.

This is, as far as I know, not a standardized file descriptor (like stdin/stdout/stderr), but rather something made up by tree. It is defined (in the tree source code) to 3. treeonly checks for this file descriptor, it does not verify its type or perform any other sanity checks.

I don't use tree nor am I familiar with it's codebase, but what you're describing sounds like it is something that should be addressed by tree's upstream rather than us trying to hack around it.

[dnkl]

I certainly don't agree with tree's implementation.

But with that said, you never exec other programs with file-descriptors other than stdin/stdout/stderr open. Unless it's intended. I.e. you are passing an FD to the child on purpose. But this; an inotify FD used by nnn itself, should not be passed to the shell.

Closing your own file-descriptors before exec:ing a child is not "hacking around it". It's what you're supposed to do.

[N-R-K]

@dnkl I didn't pay attention to your other post and was under the impression you were proposing using that bash script as a solution. Sorry for the confusion.

[dnkl]

No worries, and I hope I didn't come off as being rude. The bash script was only meant as a way to show/prove that the leaked FD is the cause, and I could have been more clear about that.

[badxnach]

Thank you very much for checking this in such depth, dnkl.
Can we do something about this issue? I use tree quite often to make directory listings of external hdds.

[dnkl]

imho, nnn needs to be patched.

Not sure if it leaks other file descriptors as well; one can either inspect the nnn source code, and ensure all opened file descriptors have CLOEXEC set. Or, nnn can loop, and just "try" to close all file descriptors in, say, the range 3-1024 after having forked, but before exec:ing a process.

[N-R-K]

Making thousands of syscall in a loop sounds like a really bad idea. Using CLOEXEC everywhere is the probably the proper solution here.

I'm at the end of my awake hours though. We can pick this up tomorrow, or someone else wants to take a crack then feel free to do so.

@jarun

[N-R-K]

@badxnach @dnkl does this fix the issue with tree?

diff --git a/src/nnn.c b/src/nnn.c
index a0fafe9..a6491bd 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -8629,7 +8629,7 @@ int main(int argc, char *argv[])
 
 #ifdef LINUX_INOTIFY
 	/* Initialize inotify */
-	inotify_fd = inotify_init1(IN_NONBLOCK);
+	inotify_fd = inotify_init1(IN_NONBLOCK | IN_CLOEXEC);
 	if (inotify_fd < 0) {
 		xerror();
 		return EXIT_FAILURE;

Skimmed through most of the open() calls and they appear to be short lived, don't think they should leak to the plugins.

---

Also just out of curiosity, any clue on what's going on here with kitty? @dnkl

Tried kitty, it's fine, so it's foot that's the issue.

[badxnach]

Can cofirm that this fixes the issue on my end.
I checked tree, preview-tui and two of my own plugins. All work as they should.
Thank you very much, @N-R-K and @dnkl for investigating and solving this.

[dnkl]

Patch works for me as well, thanks! Regarding kitty; I've no idea - for me, tree breaks in kitty as well, in exactly the same way (and is fixed by the patch).

[dnkl]

Kitty does loop and close FDs in the range 3-200 when exec:ing its child process. But that means it only applies to the first shell instance; when nnn runs, and when nnn exec's a plugin, kitty is not involved in the process, and no FDs are closed. Unless nnn spawns a new kitty instance when you start a shell from it (it doesn't for me).

[duffydack]

on arch, tree 2.0.2-1 fixes the issue. tree changelog,

Version 2.0.2 (02/16/2022)
Okay, apparently the stddata addition is causing havoc (who knew how many scripts just haphazardly hand programs random file descriptors, that's surely not a problem.) Going forward the stddata option will only work if the environment variable STDDATA_FD is present or set to the descriptor to produce the JSON output on.

[N-R-K]

Going forward the stddata option will only work if the environment variable STDDATA_FD is present or set to the descriptor to produce the JSON output on.

That's definitely the right move. 😄