search

SirVer / ultisnips

UltiSnips - The ultimate snippet solution for Vim. Send pull requests to SirVer/ultisnips!
GNU General Public License v3.0
7.55k stars 691 forks source link

Sanitize null bytes in eval'ed text #1538

Closed yut23 closed 1 year ago

yut23 commented 1 year ago

Both vim and neovim error out if we try to evaluate a string with a null byte in it. It appears that they use <NL> to represent <Nul> in several places, so replace any null bytes in text passed to vim_helper.eval() with newlines.

In my testing, I copied text from a file containing a null byte and pasted it into the commands that were giving errors, and found that the null byte does get treated as a word boundary in that case: https://github.com/SirVer/ultisnips/blob/24a3ebb36687b6d59a19d63173713575b486d739/pythonx/UltiSnips/snippet/definition/base.py#L345-L347 https://github.com/SirVer/ultisnips/blob/24a3ebb36687b6d59a19d63173713575b486d739/pythonx/UltiSnips/snippet/definition/base.py#L388-L392

Fixes #1386.

yut23 commented 1 year ago

I'll see if I can add some tests tomorrow.

SirVer commented 1 year ago

Thank you for your contribution! A test would indeed be highly valued.

One question: Why newline though? Why not space or empty text? Is there a rational for the one over the other?

yut23 commented 1 year ago

Ah, it looks like markdown interpreted the character names as html tags and hid them. See :h NL-used-for-Nul/:helpg <Nul> for more details. I think a space would also work fine for the commands that were giving errors, but I figured it would be better to be able to distinguish between actual spaces and null bytes in general. I know snippet triggers can have spaces, but I don't think they can have multiple lines.

I've been pretty busy this week, but I should be able to add tests this weekend.

yut23 commented 1 year ago

I've added some tests and checked that they fail without this change (https://github.com/yut23/ultisnips/pull/1). I also found that any failing CI tests show up as passing in the GitHub interface, which is fixed in #1541 (I also included the commit in this PR, so it should be merged before this one).

SirVer commented 1 year ago

What an exemplary contribution to open source, thank you so much!