VS Code leaks open file descriptors to subprocesses in terminal

[ytimenkov]

We have written the needed data into your clipboard because it was too large to send. Please paste.
Issue Type: Bug

1. Open Terminal
2. Run program. For example top
3. List open files: sudo lsof -p $(/sbin/pidof top)

Apparently VS Code leaks some files open without CLOEXEC flag:

top     26575 default    5r      REG              253,0  10221472  134754526 /usr/share/code/icudtl.dat
top     26575 default    6r      REG              253,0   1011384  135022222 /usr/share/code/v8_context_snapshot.bin
top     26575 default    7r      REG              253,0    125011  134878618 /usr/share/code/natives_blob.bin
top     26575 default    8r      REG              253,0    167930  134683664 /usr/share/code/chrome_100_percent.pak
top     26575 default    9r      REG              253,0    250097  134689839 /usr/share/code/chrome_200_percent.pak
top     26575 default   10r      REG              253,0     58454     140353 /usr/share/code/locales/en-US.pak
top     26575 default   11r      REG              253,0   8690944  134878619 /usr/share/code/resources.pak

Plus some chromium stuff like fonts and files on /dev/shm

VS Code version: Code 1.37.1 (f06011ac164ae4dc8e753a3fe7f9549844d15e35, 2019-08-15T16:17:25.463Z)
OS version: Linux x64 3.10.0-957.10.1.el7.x86_64

System Info

Item	Value
CPUs	Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz (6 x 3191)
GPU Status	2d_canvas: unavailable_software flash_3d: disabled_software flash_stage3d: disabled_software flash_stage3d_baseline: disabled_software gpu_compositing: disabled_software multiple_raster_threads: enabled_on native_gpu_memory_buffers: disabled_software oop_rasterization: disabled_off protected_video_decode: disabled_off rasterization: disabled_software skia_deferred_display_list: disabled_off skia_renderer: disabled_off surface_synchronization: enabled_on video_decode: disabled_software viz_display_compositor: disabled_off webgl: unavailable_software webgl2: unavailable_software
Load (avg)	1, 0, 0
Memory (System)	49.50GB (25.83GB free)
Process Argv	--unity-launch
Screen Reader	no
VM	100%

Extensions (12)

Extension	Author (truncated)	Version
language-gas-x86	bas	0.0.1
gitlens	eam	9.9.3
rest-client	hum	0.22.0
vscode-docker	ms-	0.7.0
vscode-kubernetes-tools	ms-	1.0.4
python	ms-	2019.8.30787
cpptools	ms-	0.25.1
powershell	ms-	2019.5.0
vscode-yaml	red	0.5.2
highlight-words	rsb	0.1.4
jinjahtml	sam	0.10.5
vscode-icons	vsc	9.3.0

(2 theme extensions excluded)

[kentonv]

Today I found vscode had leaked 168 file descriptors into my terminal, pointing to lots of Chromium shared memory segments, V8 snapshot, fonts, other static assets, etc.

I think its important that vscode fix this by closing all FDs greater than 2 before exec'ing the terminal. Developers commonly run buggy work-in-progress code in their terminal, and it would be pretty easy for buggy code to accidentally write to one of these FDs causing who-knows-what effects to vscode.

As a work-around I can close all unexpected FDs by executing this command:

for fd in $(ls /proc/$$/fd); do if [ $fd -ge 3 ] && [ $fd != 255 ]; then eval "exec $fd>&-"; fi; done

(Note that in addition to stdin/stdout/stderr, this keeps fd 255 open. Bash apparently stores a dup of the terminal into fd 255 and uses it for various things, so if you close it, it causes trouble.)

[Tyriar]

FYI @deepak1556

/duplicate microsoft/vscode#58814

[kentonv]

@Tyriar It appears microsoft/vscode#58814 was closed on the basis that this is not a security bug. I mostly agree with that, however, it is still most certainly a bug.

Leaving file descriptors in the FD table when spawning subprocesses is considered bad behavior. At best it's sloppy, but at worst it can cause problems.

Some examples of problems:

• If the user starts a daemon from the terminal, that daemon might hold on to those file descriptors long after vscode is shut down, possibly consuming resources that should have been cleaned up.
• If the user starts some sort of program that sets up a container-based sandbox, it's possible that program does not expect to inherit random file descriptors and won't properly close them, thus allowing the FDs to be accessed inside the sandbox. This is arguably a bug in the container implementation, but it's a common bug that wouldn't normally be a problem since most TTYs don't have random file descriptors laying around.
• I myself am working on a system that creates secure containers. My code is careful to close all unexpected file descriptors. However, since my file descriptor management is relatively complex, I have some debugging code that complains if it sees leaked file descriptors, because it assumes this is a bug in my own code. When I run my code in a vscode terminal, it always complains about leaked FDs, but it turns out this is not a bug in my code, it's vscode's leak. This is annoying.

While the ideal solution would be for vscode to open file descriptors with O_CLOEXEC, a relatively easy alternative would be for you to close all file descriptors >= 3 after fork() but before exec(). You could literally just loop through integers 3 to 1024* and call close() on each one. Or, you could list /proc/self/fd and close all FD numbers >= 3 seen there.

* File descriptor numbers aren't limited to 1024, but the OS always uses the lowest possible number for any new descriptor, so it's unusual for them to get very high.

[ytimenkov]

I do not remember in which context I opened this issue and if it was a real problem... Maybe since I work with shared memory in app it was confusing to get extra open descriptors and harder to debug.

Just a comment about closing in a loop: I think kernel has a specific syscall for it or close_range libc function.

[kentonv]

close_range() was introduced in Linux 5.9, i.e. very recently. It may be premature to depend on it, but it's certainly a convenient answer when it's available.

[Tyriar]

@kentonv this is a bit out of my depth tbh but would welcome a PR to https://github.com/microsoft/node-pty to fix it as I think that's where the fix would go.

[jerch]

@Tyriar @Eugeny already addressed this issue in #487.