Skip to content

fix: call recursive_delete/recursive_copy callback only once - #788

Merged
stevearc merged 1 commit into
stevearc:masterfrom
kenjiThiago:fix/recursive-fs-double-callback
Oct 12, 2026
Merged

stevearc merged 1 commit into
stevearc:masterfrom
kenjiThiago:fix/recursive-fs-double-callback

Conversation

@kenjiThiago

Copy link
Copy Markdown
Contributor

Deleting a large directory such as node_modules from oil can abort Neovim with
free(): double free detected in tcache 2. This is the crash reported in #719, and
probably the one in #761.

recursive_delete removes all entries of a directory batch at the same time. With the
default soft limit of 1024 open files, a tree with a few thousand directories makes
fs_opendir fail with EMFILE for many of them at once. The first error reaches
complete, which replaces itself with a no-op and calls inner_cb. Replacing it only
changes the local variable: each child operation already holds the original function, so
every later error calls inner_cb again, and each call runs uv.fs_closedir(fd) on the
same handle.

The no-op reassignment never worked, but before #712 it was unreachable: complete
checked err instead of err2, so child errors were ignored. recursive_copy uses the
same pattern. It doesn't crash because it never closes the directory, though it still
calls its callback more than once.

This change adds a failed flag shared by all reads of the directory, so inner_cb runs
once and later errors are ignored.

Reproduction

Two failures in the same directory are enough, so permissions give a simpler repro than
the file limit:

mkdir -p /tmp/repro/locked
touch /tmp/repro/locked/a /tmp/repro/locked/b
chmod a-w /tmp/repro/locked
nvim /tmp

Delete repro/ in oil and save. Without this change Neovim aborts; with it, oil shows a
single EACCES error and Neovim keeps running. A single failing entry was already
reported correctly.

With ulimit -Sn 1024, deleting a big node_modules now fails with one EMFILE error
instead of crashing. Oil still can't remove the whole tree in one pass, because it opens
directories with no concurrency limit.

Fixes #719

@github-actions
github-actions Bot requested a review from stevearc October 11, 2026 05:34
@stevearc

Copy link
Copy Markdown
Owner

LGTM, thanks!

@stevearc
stevearc merged commit d3733d9 into stevearc:master Oct 12, 2026
9 checks passed
@kenjiThiago

kenjiThiago commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for merging! With this fix a big delete no longer crashes, but it can still fail partway with EMFILE when the open file limit is 1024 (the systemd default). recursive_delete recurses into all entries at once and keeps each directory open until its children finish, so a large tree like node_modules ends up with more than 1024 directories open.

I have a version on my fork that reads each directory fully and closes it before going deeper, with a cap on how many are open at once. It deletes a 300 MB node_modules without errors, and I've been using it locally. I want to run a few more tests first, but would you be interested in a PR for this?

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.

bug: neovim crash pointer being freed was not allocated on delete node_modules

2 participants