fix(path): avoid chdir() when resolving path #28799

zeertzjq · 2024-05-17T01:24:00Z

Use uv_fs_realpath() instead.

It seems that uv_fs_realpath() has some problems on non-Linux platforms:

        This function has certain platform-specific caveats that were discovered when used in Node.

        * macOS and other BSDs: this function will fail with UV_ELOOP if more than 32 symlinks are
          found while resolving the given path.  This limit is hardcoded and cannot be sidestepped.
        * Windows: while this function works in the common case, there are a number of corner cases
          where it doesn't:

          - Paths in ramdisk volumes created by tools which sidestep the Volume Manager (such as ImDisk)
            cannot be resolved.
          - Inconsistent casing when using drive letters.
          - Resolved path bypasses subst'd drives.

        While this function can still be used, it's not recommended if scenarios such as the
        above need to be supported.

        The background story and some more details on these issues can be checked
        `here <https://github.com/nodejs/node/issues/7726>`_.

I don't know if the old implementation that uses uv_chdir() and uv_cwd() also suffers from the same problems.

For the ELOOP case, chdir() seems to have the same limitations as realpath().
On Windows, Vim doesn't use anything like chdir() either. It uses _wfullpath(), while libuv uses GetFinalPathNameByHandleW().

Use uv_fs_realpath() instead. It seems that uv_fs_realpath() has some problems on non-Linux platforms: - macOS and other BSDs: this function will fail with UV_ELOOP if more than 32 symlinks are found while resolving the given path. This limit is hardcoded and cannot be sidestepped. - Windows: while this function works in the common case, there are a number of corner cases where it doesn't: - Paths in ramdisk volumes created by tools which sidestep the Volume Manager (such as ImDisk) cannot be resolved. - Inconsistent casing when using drive letters. - Resolved path bypasses subst'd drives. Ref: https://docs.libuv.org/en/v1.x/fs.html#c.uv_fs_realpath I don't know if the old implementation that uses uv_chdir() and uv_cwd() also suffers from the same problems. - For the ELOOP case, chdir() seems to have the same limitations. - On Windows, Vim doesn't use anything like chdir() either. It uses _wfullpath(), while libuv uses GetFinalPathNameByHandleW().

justinmk

Great solution. The os_chdir approach is from vim but they must have changed their approach at some point. Maybe we can mark some patches as N/A or merged, after this.

zeertzjq · 2024-05-17T10:26:59Z

I think Vim has always used the chdir() approach on Unix, but not on Windows.

github-actions · 2024-05-17T10:39:19Z

Successfully created backport PR for release-0.10:

fix(path): avoid chdir() when resolving path #28808

Use uv_fs_realpath() instead. It seems that uv_fs_realpath() has some problems on non-Linux platforms: - macOS and other BSDs: this function will fail with UV_ELOOP if more than 32 symlinks are found while resolving the given path. This limit is hardcoded and cannot be sidestepped. - Windows: while this function works in the common case, there are a number of corner cases where it doesn't: - Paths in ramdisk volumes created by tools which sidestep the Volume Manager (such as ImDisk) cannot be resolved. - Inconsistent casing when using drive letters. - Resolved path bypasses subst'd drives. Ref: https://docs.libuv.org/en/v1.x/fs.html#c.uv_fs_realpath I don't know if the old implementation that uses uv_chdir() and uv_cwd() also suffers from the same problems. - For the ELOOP case, chdir() seems to have the same limitations. - On Windows, Vim doesn't use anything like chdir() either. It uses _wfullpath(), while libuv uses GetFinalPathNameByHandleW(). (cherry picked from commit 42aa69b)

Use uv_fs_realpath() instead. It seems that uv_fs_realpath() has some problems on non-Linux platforms: - macOS and other BSDs: this function will fail with UV_ELOOP if more than 32 symlinks are found while resolving the given path. This limit is hardcoded and cannot be sidestepped. - Windows: while this function works in the common case, there are a number of corner cases where it doesn't: - Paths in ramdisk volumes created by tools which sidestep the Volume Manager (such as ImDisk) cannot be resolved. - Inconsistent casing when using drive letters. - Resolved path bypasses subst'd drives. Ref: https://docs.libuv.org/en/v1.x/fs.html#c.uv_fs_realpath I don't know if the old implementation that uses uv_chdir() and uv_cwd() also suffers from the same problems. - For the ELOOP case, chdir() seems to have the same limitations. - On Windows, Vim doesn't use anything like chdir() either. It uses _wfullpath(), while libuv uses GetFinalPathNameByHandleW().

* version bump * docs: news neovim#28773 * perf(treesitter): use child_containing_descendant() in has-ancestor? (neovim#28512) Problem: `has-ancestor?` is O(n²) for the depth of the tree since it iterates over each of the node's ancestors (bottom-up), and each ancestor takes O(n) time. This happens because tree-sitter's nodes don't store their parent nodes, and the tree is searched (top-down) each time a new parent is requested. Solution: Make use of new `ts_node_child_containing_descendant()` in tree-sitter v0.22.6 (which is now the minimum required version) to rewrite the `has-ancestor?` predicate in C to become O(n). For a sample file, decreases the time taken by `has-ancestor?` from 360ms to 6ms. * feat: remove deprecated features Remove following functions: - vim.lsp.util.extract_completion_items - vim.lsp.util.get_progress_messages - vim.lsp.util.parse_snippet() - vim.lsp.util.text_document_completion_list_to_complete_items - LanguageTree:for_each_child - health#report_error - health#report_info - health#report_ok - health#report_start - health#report_warn - vim.health.report_error - vim.health.report_info - vim.health.report_ok - vim.health.report_start - vim.health.report_warn * fix(version): fix vim.version().prerelease fixes neovim#28782 (when backported) * fix: extend the life of vim.tbl_flatten to 0.13 `vim.iter(t):flatten():totable()` doesn't handle nil so isn't a good enough replacement. * docs(gen_help_html.lua): handle modeline and note nodes Problem: 'modeline' and 'note' are unhandled in the online HTML documentation. Some (not all) modelines are parsed by the vimdoc parser as a node of type 'modeline'. Solution: - Ignore 'modeline' in HTML rendering. - Render 'note' text in boldface. * fix(health): broken ruby detect neovim#28804 * fix(path): avoid chdir() when resolving path (neovim#28799) Use uv_fs_realpath() instead. It seems that uv_fs_realpath() has some problems on non-Linux platforms: - macOS and other BSDs: this function will fail with UV_ELOOP if more than 32 symlinks are found while resolving the given path. This limit is hardcoded and cannot be sidestepped. - Windows: while this function works in the common case, there are a number of corner cases where it doesn't: - Paths in ramdisk volumes created by tools which sidestep the Volume Manager (such as ImDisk) cannot be resolved. - Inconsistent casing when using drive letters. - Resolved path bypasses subst'd drives. Ref: https://docs.libuv.org/en/v1.x/fs.html#c.uv_fs_realpath I don't know if the old implementation that uses uv_chdir() and uv_cwd() also suffers from the same problems. - For the ELOOP case, chdir() seems to have the same limitations. - On Windows, Vim doesn't use anything like chdir() either. It uses _wfullpath(), while libuv uses GetFinalPathNameByHandleW(). * feat(api): broadcast events to ALL channels neovim#28487 Problem: `vim.rpcnotify(0)` and `rpcnotify(0)` are documented as follows: If {channel} is 0, the event is broadcast to all channels. But that's not actually true. Channels must call `nvim_subscribe` to receive "broadcast" events, so it's actually "multicast". - Assuming there is a use-case for "broadcast", the current model adds an extra step for broadcasting: all channels need to "subscribe". - The presence of `nvim_subscribe` is a source of confusion for users, because its name implies something more generally useful than what it does. Presumably the use-case of `nvim_subscribe` is to avoid "noise" on RPC channels not expected a broadcast notification, and potentially an error if the channel client reports an unknown event. Solution: - Deprecate `nvim_subscribe`/`nvim_unsubscribe`. - If applications want to multicast, they can keep their own multicast list. Or they can use `nvim_list_chans()` and `nvim_get_chan_info()` to enumerate and filter the clients they want to target. - Always send "broadcast" events to ALL channels. Don't require channels to "subscribe" to receive broadcasts. This matches the documented behavior of `rpcnotify()`. * vim-patch:9.1.0414: Unable to leave long line with 'smoothscroll' and 'scrolloff' Problem: Unable to leave long line with 'smoothscroll' and 'scrolloff'. Corrupted screen near the end of a long line with 'scrolloff'. (Ernie Rael, after 9.1.0280) Solution: Only correct cursor in case scroll_cursor_bot() was not itself called to make the cursor visible. Avoid adjusting for 'scrolloff' beyond the text line height (Luuk van Baal) vim/vim@b32055e vim-patch:9.1.0416: some screen dump tests can be improved Problem: some screen dump tests can be improved (after 9.1.0414) Solution: Make sure screen state changes properly and is captured in the screen dumps (Luuk van Baal) vim/vim@2e64273 * fix(vim.iter): enable optimizations for arrays (lists with holes) (neovim#28781) The optimizations that vim.iter uses for array-like tables don't require that the source table has no holes. The only thing that needs to change is the determination if a table is "list-like": rather than requiring consecutive, integer keys, we can simply test for (positive) integer keys only, and remove any holes in the original array when we make a copy for the iterator. * ci: change label `backport` to `target:release` `backport` is too similar `ci:backport release-x.y` and causes confusion. * fix(move): half-page scrolling with resized grid at eob (neovim#28821) * vim-patch:9.1.0418: Cannot move to previous/next rare word (neovim#28822) Problem: Cannot move to previous/next rare word (Colin Kennedy) Solution: Add the ]r and [r motions (Christ van Willegen) fixes: vim/vim#14773 closes: vim/vim#14780 vim/vim@8e4c4c7 Co-authored-by: Christ van Willegen - van Noort <github.com@vanwillegen-vannoort.nl> * vim-patch:cf78d0df51f2 runtime(sshdconfig): add basic ftplugin file for sshdconfig (vim/vim#14790) vim/vim@cf78d0d Co-authored-by: Yinzuo Jiang <jiangyinzuo@foxmail.com> * vim-patch:94043780196c (neovim#28831) runtime(matchparen): fix :NoMatchParen not working (vim/vim#14797) fixes: neovim#28828 vim/vim@9404378 * refactor(path.c): add nonnull attributes (neovim#28829) This possibly fixes the coverity warning. * refactor!: remove `nvim` and `provider` module for checkhealth The namespacing for healthchecks for neovim modules is inconsistent and confusing. The completion for `:checkhealth` with `--clean` gives ``` nvim provider.clipboard provider.node provider.perl provider.python provider.ruby vim.lsp vim.treesitter ``` There are now three top-level module names for nvim: `nvim`, `provider` and `vim` with no signs of stopping. The `nvim` name is especially confusing as it does not contain all neovim checkhealths, which makes it almost a decoy healthcheck. The confusion only worsens if you add plugins to the mix: ``` lazy mason nvim nvim-treesitter provider.clipboard provider.node provider.perl provider.python provider.ruby telescope vim.lsp vim.treesitter ``` Another problem with the current approach is that it's not easy to run nvim-only healthchecks since they don't share the same namespace. The current approach would be to run `:che nvim vim.* provider.*` and would also require the user to know these are the neovim modules. Instead, use this alternative structure: ``` vim.health vim.lsp vim.provider.clipboard vim.provider.node vim.provider.perl vim.provider.python vim.provider.ruby vim.treesitter ``` and ``` lazy mason nvim-treesitter telescope vim.health vim.lsp vim.provider.clipboard vim.provider.node vim.provider.perl vim.provider.python vim.provider.ruby vim.treesitter ``` Now, the entries are properly sorted and running nvim-only healthchecks requires running only `:che vim.*`. * fix(diagnostic): show backtrace for deprecation warnings Problem: On nvim 11.0-dev, deprecation warnings due to an use of hard-deprecated APIs such as: - `vim.diagnostic.disable()` - `vim.diagnostic.is_disabled()` etc. are not accompanied by backtrace information. It makes difficult for users to figure out which lines or which plugins are still using deprecated APIs. Solution: use `backtrace = true` in vim.deprecate() call. * vim-patch:df859a36d390 runtime(sql): set commentstring for sql files in ftplugin closes: vim/vim#14800 vim/vim@df859a3 Co-authored-by: Riley Bruins <ribru17@hotmail.com> * vim-patch:36e974fdf3f5 runtime(graphql): basic ftplugin file for graphql closes: vim/vim#14801 vim/vim@36e974f Co-authored-by: Riley Bruins <ribru17@hotmail.com> * vim-patch:4d7892bfb1db runtime(dart): add basic dart ftplugin file fixes vim/vim#14793 closes vim/vim#14802 vim/vim@4d7892b Co-authored-by: Riley Bruins <ribru17@hotmail.com> * vim-patch:9.1.0421: filetype: hyprlang files are not recognized Problem: filetype: hyprlang files are not recognized Solution: recognize 'hypr{land,paper,idle,lock}.conf' files as 'hyprlang' filetype, add hyprlang ftplugin (Riley Bruins) closes: vim/vim#14803 vim/vim@5f1b115 Co-authored-by: Riley Bruins <ribru17@hotmail.com> * Update CMakeLists.txt * Create health.lua --------- Co-authored-by: Justin M. Keyes <justinkz@gmail.com> Co-authored-by: vanaigr <vanaigranov@gmail.com> Co-authored-by: dundargoc <gocdundar@gmail.com> Co-authored-by: bfredl <bjorn.linse@gmail.com> Co-authored-by: Lewis Russell <lewis6991@gmail.com> Co-authored-by: Jongwook Choi <wookayin@gmail.com> Co-authored-by: MoonFruit <dkmoonfruit@gmail.com> Co-authored-by: zeertzjq <zeertzjq@outlook.com> Co-authored-by: Luuk van Baal <luukvbaal@gmail.com> Co-authored-by: Gregory Anders <8965202+gpanders@users.noreply.github.com> Co-authored-by: Christ van Willegen - van Noort <github.com@vanwillegen-vannoort.nl> Co-authored-by: Christian Clason <c.clason@uni-graz.at> Co-authored-by: Yinzuo Jiang <jiangyinzuo@foxmail.com> Co-authored-by: Riley Bruins <ribru17@hotmail.com>

justinmk · 2024-05-21T12:32:15Z

coverity:

*** CID 493871:  Resource leaks  (RESOURCE_LEAK)
/src/nvim/path.c: 2280 in path_full_dir_name()
2274     int path_full_dir_name(char *directory, char *buffer, size_t len)
2275     {
2276       if (strlen(directory) == 0) {
2277         return os_dirname(buffer, len);
2278       }
2279     
>>>     CID 493871:  Resource leaks  (RESOURCE_LEAK)
>>>     Failing to save or free storage allocated by "os_realpath(directory, buffer, len)" leaks it.
2280       if (os_realpath(directory, buffer, len) != NULL) {
2281         return OK;
2282       }
2283     
2284       // Path does not exist (yet).  For a full path fail, will use the path as-is.
2285       if (path_is_absolute(directory)) {

Use uv_fs_realpath() instead. It seems that uv_fs_realpath() has some problems on non-Linux platforms: - macOS and other BSDs: this function will fail with UV_ELOOP if more than 32 symlinks are found while resolving the given path. This limit is hardcoded and cannot be sidestepped. - Windows: while this function works in the common case, there are a number of corner cases where it doesn't: - Paths in ramdisk volumes created by tools which sidestep the Volume Manager (such as ImDisk) cannot be resolved. - Inconsistent casing when using drive letters. - Resolved path bypasses subst'd drives. Ref: https://docs.libuv.org/en/v1.x/fs.html#c.uv_fs_realpath I don't know if the old implementation that uses uv_chdir() and uv_cwd() also suffers from the same problems. - For the ELOOP case, chdir() seems to have the same limitations. - On Windows, Vim doesn't use anything like chdir() either. It uses _wfullpath(), while libuv uses GetFinalPathNameByHandleW().

zeertzjq added the filesystem file metadata/attributes, filenames, path manipulation label May 17, 2024

zeertzjq force-pushed the realpath branch from 65769af to 2f03f7a Compare May 17, 2024 01:26

zeertzjq changed the title ~~fix(path): avoid chdir() when resolving path if possible~~ fix(path): avoid chdir() when resolving path May 17, 2024

zeertzjq force-pushed the realpath branch 3 times, most recently from ebb3512 to 334470c Compare May 17, 2024 02:07

zeertzjq marked this pull request as ready for review May 17, 2024 02:07

zeertzjq force-pushed the realpath branch from 334470c to bd2e025 Compare May 17, 2024 02:58

zeertzjq changed the title ~~fix(path): avoid chdir() when resolving path~~ fix(path): avoid chdir() when resolving path if possible May 17, 2024

zeertzjq force-pushed the realpath branch 2 times, most recently from 4840ce4 to 63aa700 Compare May 17, 2024 04:05

zeertzjq changed the title ~~fix(path): avoid chdir() when resolving path if possible~~ fix(path): avoid chdir() when resolving path May 17, 2024

zeertzjq force-pushed the realpath branch from 63aa700 to 22ac881 Compare May 17, 2024 04:29

justinmk approved these changes May 17, 2024

View reviewed changes

zeertzjq added the ci:backport release-0.10 label May 17, 2024

zeertzjq added this to the 0.10.1 milestone May 17, 2024

zeertzjq merged commit 42aa69b into neovim:master May 17, 2024
33 checks passed

zeertzjq deleted the realpath branch May 17, 2024 10:39

github-actions bot mentioned this pull request May 17, 2024

fix(path): avoid chdir() when resolving path #28808

Merged

dundargoc mentioned this pull request May 21, 2024

ci: try running coverity on PRs #26620

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(path): avoid chdir() when resolving path #28799

fix(path): avoid chdir() when resolving path #28799

zeertzjq commented May 17, 2024 •

edited

justinmk left a comment

zeertzjq commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024

justinmk commented May 21, 2024

fix(path): avoid chdir() when resolving path #28799

fix(path): avoid chdir() when resolving path #28799

Conversation

zeertzjq commented May 17, 2024 • edited

justinmk left a comment

Choose a reason for hiding this comment

zeertzjq commented May 17, 2024 • edited

github-actions bot commented May 17, 2024

justinmk commented May 21, 2024

zeertzjq commented May 17, 2024 •

edited

zeertzjq commented May 17, 2024 •

edited