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

Sanitize null bytes in eval'ed text #1538

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

yut23
Copy link
Contributor

@yut23 yut23 commented Sep 19, 2023

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:

# Require a word boundary between prefix and suffix.
boundary_chars = escape(words_prefix[-1:] + words_suffix[:1], r"\"")
match = vim_helper.eval('"%s" =~# "\\\\v.<."' % boundary_chars) != "0"

# Trim non-empty prefix up to word boundary, if present.
qwords = escape(words, r"\"")
words_suffix = vim_helper.eval(
'substitute("%s", "\\\\v^.+<(.+)", "\\\\1", "")' % qwords
)

Fixes #1386.

@yut23
Copy link
Contributor Author

yut23 commented Sep 19, 2023

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

@SirVer
Copy link
Owner

SirVer commented Sep 21, 2023

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
Copy link
Contributor Author

yut23 commented Sep 21, 2023

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 and others added 3 commits September 24, 2023 20:49
With `pipefail`, the exit code for a pipeline will be taken from the first failing command in the pipeline, rather than the last command.
@yut23
Copy link
Contributor Author

yut23 commented Sep 25, 2023

I've added some tests and checked that they fail without this change (yut23#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
Copy link
Owner

SirVer commented Sep 25, 2023

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

@SirVer SirVer merged commit f6d1501 into SirVer:master Sep 25, 2023
11 checks passed
@yut23 yut23 deleted the sanitize_null_bytes branch October 17, 2023 01:37
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.

Ultisnips crashes when trying to expand a snippet that starts with ^@
2 participants