Skip to content

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

Closed
bobrippling wants to merge 2 commits into
vim:masterfrom
bobrippling:fix/modify-fname
Closed

Fix fnamemodify()'s handling of :r after :e#5024
bobrippling wants to merge 2 commits into
vim:masterfrom
bobrippling:fix/modify-fname

Conversation

@bobrippling

@bobrippling bobrippling commented Oct 6, 2019

Copy link
Copy Markdown
Contributor

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)

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

Copy link
Copy Markdown
Contributor Author

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
manuelschiller pushed a commit to manuelschiller/vim that referenced this pull request Nov 10, 2019
Problem:    Fnamemodify() fails when repeating :e.
Solution:   Do not go before the tail. (Rob Pilling, closes vim#5024)
@bobrippling bobrippling deleted the fix/modify-fname branch June 29, 2021 21:29
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.

1 participant