Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fnamemodify()'s handling of :r after :e #5024

Closed
wants to merge 2 commits into from

Conversation

@bobrippling
Copy link

commented Oct 6, 2019

During fnamemodify(), for ":r" we will iterate up the filename. Ensuring that we don't go before the filename's first directory separator (the tail) is insufficient in cases where we've already handled a ":e" modifier, for example:

"path/to/this.file.ext" :e:e:r:r
         ^    ^-------- *fnamep
         +------------- tail

This means for a ":r", we'll go before *fnamep, and outside the bounds of the filename. This is both incorrect and causes vim to attempt to allocate a lot of memory, which will either fails and we'll continue with a null string, or we'll get a runtime allocation error.

The large memory allocation comes from calculating s - *fnamep. Since s is before *fnamep, we calculate a negative length, which ends up being interpreted as an amount to allocate, causing the above problem.

We must instead ensure we don't go before *fnamep nor tail. The check for tail is still relevant, for example, here we don't want to go before it:

"path/to/this.file.ext" :r:r:r
 ^       ^------------- tail
 +--------------------- *fnamep

(This is cloned this PR to neovim)

bobrippling added 2 commits Oct 6, 2019
During `fnamemodify()`, for `":r"` we will iterate up the filename.
Ensuring that we don't go before the filename's first directory
separator (the tail) is insufficient in cases where we've already
handled a `":e"` modifier, for example:

```
"path/to/this.file.ext" :e:e:r:r
         ^    ^-------- *fnamep
         +------------- tail
```

This means for a `":r"`, we'll go before `*fnamep`, and outside the
bounds of the filename. This is both incorrect and causes vim to attempt
to allocate a lot of memory, which will either fails and we'll continue
with a null string, or we'll get a runtime allocation error.

The large memory allocation comes from calculating `s - *fnamep`. Since
`s` is before `*fnamep`, we caluclate a negative length, which ends up
being interpreted as an amount to allocate, causing the above problem.

We must instead ensure we don't go before `*fnamep` nor `tail`. The
check for `tail` is still relevant, for example, here we don't want to
go before it:

```
"path/to/this.file.ext" :r:r:r
 ^       ^------------- tail
 +--------------------- *fnamep
```
@bobrippling bobrippling force-pushed the bobrippling:fix/modify-fname branch from d761aef to 8d61b5d Oct 7, 2019
@bobrippling

This comment has been minimized.

Copy link
Author

commented Oct 7, 2019

If I'm reading the report right, I appear to have changed test coverage to uncover some lines in src/if_xcmdsrv.c - I see this on a few other PRs so I'm guessing my changes are okay (since there's no other automated check issues)

@brammool brammool closed this in b189295 Oct 8, 2019
justinmk added a commit to neovim/neovim that referenced this pull request Oct 11, 2019
Problem:    Fnamemodify() fails when repeating :e.
Solution:   Do not go before the tail. (Rob Pilling, closes vim/vim#5024)
vim/vim@b189295
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.