[RDY] Fix crash on invalid modelines #5450

florolf · 2016-10-08T16:04:40Z

justinmk · 2016-10-08T16:21:58Z

src/nvim/charset.c

@@ -1740,11 +1759,8 @@ char_u* skiptowhite_esc(char_u *p) {
 /// @return Number read from the string.
 intmax_t getdigits(char_u **pp)
 {
-  errno = 0;
-  intmax_t number = strtoimax((char *)*pp, (char **)pp, 10);
-  if (number == INTMAX_MAX || number == INTMAX_MIN) {


See a371f10 and #5286 (comment) for why this check was here.

The check for INTMAX_MAX/INTMAX_MIN is a little redundant if we reset errno before. On the other hand, a strict reading of strtoimax(3p) would theoretically allow an implementation to set errno to ERANGE even if an overflow didn't occur (returning a value other than the extremal ones). I'll re-add the check.

jamessan · 2016-10-08T18:28:56Z

There are some lint failures:

src/nvim/buffer.c:4536:  if should always use braces  [readability/braces] [5]

src/nvim/buffer.c:4539:  if should always use braces  [readability/braces] [5]

src/nvim/charset.c:1745:  Missing space before asterisk in )*  [whitespace/operators] [2]

src/nvim/charset.c:1747:  Boolean operator should be placed on the same line as the start of its right operand  [whitespace/operators] [4]

Could you also add a test?

oni-link · 2016-10-11T20:25:43Z

src/nvim/charset.c

-    assert(errno != ERANGE);
-  }
+  intmax_t number;
+  assert(getdigits_safe(pp, &number) == 0);


Number is only set in debug mode.

Yay, QB saw that too (for those days when oni-link's scrutiny isn't present).

Good catch, thanks.

I'm currently traveling and I'll address all the comments once I'm back.

justinmk · 2016-10-16T20:35:58Z

src/nvim/charset.c

+
+  // prevent "unused variable" warning if we are not doing a debug build
+  (void) ret;
+  assert(ret == 0);


to avoid the unused variable warning, could do instead

if (ret != 0) { abort(); }

But then the abort would even trigger in builds which don't enable asserts. Not sure if that's okay.

It's better than a silent bug, usually

justinmk · 2016-10-16T20:39:35Z

src/nvim/charset.c

+    return -1;
+  }
+
+  return 0;


Returning OK/FAIL or true/false would be more idiomatic for the project I believe. Also seems to be what other functions in charset.c are doing.

The other functions which return true/false are predicates, as far as I can tell. Using booleans for reporting failures always seemed a little "un-C-ish" to me, but I don't know the neovim codebase well enough. If you think otherwise, I'll change it.

OK/FAIL/NOTDONE (from vim.h) is not a boolean, any objection to that?

Most important is to try to keep the pattern the same. OK=1, if we return 0 in some places to mean "ok", that can be unsettling.

Okay, sounds sensible.

Add a version of getdigits which does not crash on overflows, but reports them to the caller and reimplement getdigits using that function.

A file containing the string "vim" followed by a very large number in a location that can contain modelines will trigger an overflow in getdigits() which is called by chk_modeline() when trying to parse the version number. Replace getdigits() by getdigits_safe() to avoid this issue.

While this PR didn't directly touch this line, the linter started complaining about it as a result. Fix it nonetheless.

vim used to crash when the version number given in a mode line was too large. Add a regression test for that scenario.

jamessan · 2016-10-20T15:33:51Z

src/nvim/buffer.c

@@ -4528,16 +4528,20 @@ chk_modeline (
          e = s + 4;
        else
          e = s + 3;
-        vers = getdigits_int(&e);
+        if (getdigits_safe(&e, &vers) < 0) {


This is a change in behavior from Vim. This causes nvim to skip the modeline entirely, where as Vim ends up getting vers = -1 since it downcasts the long to an int and then uses that in the comparison on line 4539. The comparison succeeds and the modeline is set.

Now, one can question whether that's the correct behavior for Vim (and I would suggest bringing it up there), but that still leaves open how nvim should behave. I'm in favor of ignoring the modeline, since I think it's better to be overcautious with modelines, but we should also be aware of and intentional about places where we deviate in behavior from Vim.

I agree the modeline should be ignored, because most likely in that case it was not intended to be a modeline (or it's intended to be a very large version which will never match).

Also regarding the specific code here, it should check != OK. I fixed this during the merge.

justinmk · 2016-10-26T11:12:11Z

Merged. Thanks @florolf

Closes #5449 A file containing the string "vim" followed by a very large number in a modeline location will trigger an overflow in getdigits() which is called by chk_modeline() when trying to parse the version number. Add getdigits_safe(), which does not assert overflows, but reports them to the caller.

FEATURES: 0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu c6ac4f8 neovim#4934 API: call any API method from vimscript 31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number() e7e2844 has("nvim-1.2.3") checks for a specific Nvim version 522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance 719dae2 neovim#5384 events: allow event processing in getchar() f25797f neovim#5386 API: metadata: Nvim version & API level 22dfe69 neovim#5389 API: metadata: "since", "deprecated_since" 605e743 Added QuickFixLine highlight group CHANGES: 4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline() 6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode. 9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods. eeec0ca neovim#5419 tui: Default to normal-mode cursor shape. FIXES: e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes" 10a54ad neovim#5243 signal_init: Always unblock SIGCHLD. bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job 626065d neovim#5227 tchdir: New tab should inherit CWD. cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid. 6127eae shada: Fix non-writeable ShaDa directory handling ca65514 neovim#2789 system(): Respect shellxescape, shellxquote 2daf54e neovim#4874 Restore vim-like tab dragging 0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names. 3c53371 neovim#4972 from justinmk/schedule-ui_refresh 68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown. c8b6ec2 neovim#5409 v:count broken in command-line window 6bc3bce neovim#5461 fix emoji display 51937e1 neovim#5470 fix :terminal with :argadd, :argu 79d77da neovim#5481 external UIs: opening multiple files from command-line 657ba62 neovim#5501 rplugin: resolve paths in manifest file 6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash. 1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK 2a6c5bb neovim#5450 modeline: Handle version number overflow. 0ade1bb neovim#5225 CI tests now run against Windows!

justinmk reviewed Oct 8, 2016

View reviewed changes

marvim added the RFC label Oct 8, 2016

oni-link reviewed Oct 11, 2016

View reviewed changes

justinmk reviewed Oct 16, 2016

View reviewed changes

florolf force-pushed the fix-modeline-crash branch from fe2bfa4 to 45510dd Compare October 16, 2016 21:20

florolf added 4 commits October 16, 2016 23:23

charset: add getdigits_safe

c53f8c6

Add a version of getdigits which does not crash on overflows, but reports them to the caller and reimplement getdigits using that function.

buffer: fix unrelated lint failures

122b80f

While this PR didn't directly touch this line, the linter started complaining about it as a result. Fix it nonetheless.

add functional test for issue neovim#5449

aa3a92a

vim used to crash when the version number given in a mode line was too large. Add a regression test for that scenario.

florolf force-pushed the fix-modeline-crash branch from 45510dd to aa3a92a Compare October 16, 2016 21:23

florolf changed the title ~~[RFC] Fix crash on invalid modelines~~ [RDY] Fix crash on invalid modelines Oct 17, 2016

marvim added RDY and removed RFC labels Oct 17, 2016

jamessan reviewed Oct 20, 2016

View reviewed changes

justinmk mentioned this pull request Oct 26, 2016

[RFC] Issue 5449 Added fix for modeline with number above range of MAX_INT #5533

Closed

justinmk closed this Oct 26, 2016

justinmk removed the RDY label Oct 26, 2016

Uh oh!

[RDY] Fix crash on invalid modelines #5450

[RDY] Fix crash on invalid modelines #5450

Uh oh!

Conversation

florolf commented Oct 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamessan commented Oct 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Oct 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Oct 26, 2016

Uh oh!

Uh oh!

justinmk Oct 16, 2016 •

edited

Loading

justinmk Oct 26, 2016 •

edited

Loading